Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 19 Dec 2022 14:38:17 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Arun Ramadoss <arun.ramadoss@microchip.com>
Cc:     linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        woojung.huh@microchip.com, UNGLinuxDriver@microchip.com,
        andrew@lunn.ch, vivien.didelot@gmail.com, f.fainelli@gmail.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, linux@armlinux.org.uk,
        Tristram.Ha@microchip.com, richardcochran@gmail.com,
        ceggers@arri.de
Subject: Re: [Patch net-next v4 05/13] net: dsa: microchip: ptp: enable
 interrupt for timestamping
Message-ID: <20221219123817.bhsyujqgzl5mrsxw@skbuf>
References: <20221212102639.24415-1-arun.ramadoss@microchip.com>
 <20221212102639.24415-1-arun.ramadoss@microchip.com>
 <20221212102639.24415-6-arun.ramadoss@microchip.com>
 <20221212102639.24415-6-arun.ramadoss@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221212102639.24415-6-arun.ramadoss@microchip.com>
 <20221212102639.24415-6-arun.ramadoss@microchip.com>
Precedence: bulk

On Mon, Dec 12, 2022 at 03:56:31PM +0530, Arun Ramadoss wrote:
> +int ksz_ptp_irq_setup(struct dsa_switch *ds, u8 p)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	const struct ksz_dev_ops *ops = dev->dev_ops;
> +	struct ksz_port *port = &dev->ports[p];
> +	struct ksz_irq *ptpirq = &port->ptpirq;
> +	int ret;
> +
> +	ptpirq->dev = dev;
> +	ptpirq->masked = 0;
> +	ptpirq->nirqs = 3;
> +	ptpirq->reg_mask = ops->get_port_addr(p, REG_PTP_PORT_TX_INT_ENABLE__2);
> +	ptpirq->reg_status = ops->get_port_addr(p,
> +						REG_PTP_PORT_TX_INT_STATUS__2);
> +	snprintf(ptpirq->name, sizeof(ptpirq->name), "ptp_irq-%d", p);
> +
> +	ptpirq->irq_num = irq_find_mapping(port->pirq.domain, PORT_SRC_PTP_INT);
> +	if (ptpirq->irq_num < 0)
> +		return ptpirq->irq_num;
> +
> +	ptpirq->domain = irq_domain_add_simple(dev->dev->of_node, ptpirq->nirqs,
> +					       0, &ksz_ptp_irq_domain_ops,
> +					       ptpirq);

If I look at Documentation/core-api/irq/irq-domain.rst, I read this:

| Legacy
| ------
| 
| ::
| 
| 	irq_domain_add_simple()
| 	irq_domain_add_legacy()
| 	irq_domain_create_simple()
| 	irq_domain_create_legacy()
| 
| The Legacy mapping is a special case for drivers that already have a
| range of irq_descs allocated for the hwirqs.  It is used when the
| driver cannot be immediately converted to use the linear mapping.  For
| example, many embedded system board support files use a set of #defines
| for IRQ numbers that are passed to struct device registrations.  In that
| case the Linux IRQ numbers cannot be dynamically assigned and the legacy
| mapping should be used.
| 
| As the name implies, the \*_legacy() functions are deprecated and only
| exist to ease the support of ancient platforms. No new users should be
| added. Same goes for the \*_simple() functions when their use results
| in the legacy behaviour.
| 
| The legacy map assumes a contiguous range of IRQ numbers has already
| been allocated for the controller and that the IRQ number can be
| calculated by adding a fixed offset to the hwirq number, and
| visa-versa.  The disadvantage is that it requires the interrupt
| controller to manage IRQ allocations and it requires an irq_desc to be
| allocated for every hwirq, even if it is unused.
| 
| The legacy map should only be used if fixed IRQ mappings must be
| supported.  For example, ISA controllers would use the legacy map for
| mapping Linux IRQs 0-15 so that existing ISA drivers get the correct IRQ
| numbers.
| 
| Most users of legacy mappings should use irq_domain_add_simple() or
| irq_domain_create_simple() which will use a legacy domain only if an IRQ range
| is supplied by the system and will otherwise use a linear domain mapping.
| The semantics of this call are such that if an IRQ range is specified then
| descriptors will be allocated on-the-fly for it, and if no range is
| specified it will fall through to irq_domain_add_linear() or
| irq_domain_create_linear() which means *no* irq descriptors will be allocated.

I think you should be absolutely fine with using irq_domain_add_linear().

> +	if (!ptpirq->domain)
> +		return -ENOMEM;
> +
> +	ret = request_threaded_irq(ptpirq->irq_num, NULL, ksz_ptp_irq_thread_fn,
> +				   IRQF_ONESHOT, ptpirq->name, ptpirq);
> +	if (ret)
> +		goto out;
> +
> +	ret = ksz_ptp_msg_irq_setup(port);
> +	if (ret)
> +		goto out;

I think the error path is a bit shaky. Since request_threaded_irq() is
not the devres variant (devm_request_threaded_irq()), there should be a
free_irq() call on error.

> +
> +	return 0;
> +
> +out:
> +	irq_dispose_mapping(ptpirq->irq_num);
> +
> +	return ret;
> +}