Received-SPF: pass (google.com: domain of linux-kernel+bounces-47035-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Subject: Re: [PATCH net-next v5 08/15] net: ravb: Move the IRQs
 getting/requesting in the probe() method
To: Claudiu <claudiu.beznea@tuxon.dev>, <davem@davemloft.net>,
	<edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
	<richardcochran@gmail.com>, <p.zabel@pengutronix.de>,
	<geert+renesas@glider.be>
CC: <netdev@vger.kernel.org>, <linux-renesas-soc@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, Claudiu Beznea
	<claudiu.beznea.uj@bp.renesas.com>
References: <20240131084133.1671440-1-claudiu.beznea.uj@bp.renesas.com>
 <20240131084133.1671440-9-claudiu.beznea.uj@bp.renesas.com>
From: Sergey Shtylyov <s.shtylyov@omp.ru>
Organization: Open Mobile Platform
Message-ID: <5536e607-e03b-f38e-2909-a6f6a126ff0d@omp.ru>
Date: Wed, 31 Jan 2024 22:51:28 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20240131084133.1671440-9-claudiu.beznea.uj@bp.renesas.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-KSE-Attachment-Filter-Triggered-Filters: Clean
X-KSE-BulkMessagesFiltering-Scan-Result: InTheLimit

   I said the subject needs to be changed to "net: ravb: Move getting/requesting IRQs in
the probe() method", not "net: ravb: Move IRQs getting/requesting in the probe() method".
That's not very proper English, AFAIK! =)

On 1/31/24 11:41 AM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The runtime PM implementation will disable clocks at the end of
> ravb_probe(). As some IP variants switch to reset mode as a result of
> setting module standby through clock disable APIs, to implement runtime PM
> the resource parsing and requesting are moved in the probe function and IP
> settings are moved in the open function. This is done because at the end of
> the probe some IP variants will switch anyway to reset mode and the
> registers content is lost. Also keeping only register settings operations
> in the ravb_open()/ravb_close() functions will make them faster.
> 
> Commit moves IRQ requests to ravb_probe() to have all the IRQs ready when
> the interface is open. As now IRQs getting/requesting are in a single place

   Again, "getting/requesting IRQs is done"...

> there is no need to keep intermediary data (like ravb_rx_irqs[] and
> ravb_tx_irqs[] arrays or IRQs in struct ravb_private).
> 
> In order to avoid accessing the IP registers while the IP is runtime
> suspended (e.g. in the timeframe b/w the probe requests shared IRQs and
> IP clocks are enabled) in the interrupt handlers were introduced

   But can't we just request our IRQs after we call pm_runtime_resume_and_get()?
We proaobly can... but then again, we call pm_runtime_put_sync() in the remove()
method before the IRQs are freed...  So, it still seems OK that we're adding
pm_runtime_active() calls to the IRQ handlers in this very patch, not when we'll
start calling the RPM APIs in the ndo_{open|close}() methods, correct? :-)

> pm_runtime_active() checks. The device runtime PM usage counter has been
> incremented to avoid disabling the device's clocks while the check is in
> progress (if any).
> 
> This is a preparatory change to add runtime PM support for all IP variants.
> 
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index e70c930840ce..f9297224e527 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -1092,11 +1082,23 @@ static irqreturn_t ravb_emac_interrupt(int irq, void *dev_id)
>  {
>  	struct net_device *ndev = dev_id;
>  	struct ravb_private *priv = netdev_priv(ndev);
> +	struct device *dev = &priv->pdev->dev;
> +	irqreturn_t result = IRQ_HANDLED;
> +
> +	pm_runtime_get_noresume(dev);
> +

   Not sure we need en empty line here...

> +	if (unlikely(!pm_runtime_active(dev))) {
> +		result = IRQ_NONE;
> +		goto out_rpm_put;
> +	}
>  
>  	spin_lock(&priv->lock);
>  	ravb_emac_interrupt_unlocked(ndev);
>  	spin_unlock(&priv->lock);
> -	return IRQ_HANDLED;
> +
> +out_rpm_put:
> +	pm_runtime_put_noidle(dev);
> +	return result;
>  }
>  
>  /* Error interrupt handler */
> @@ -1176,9 +1178,15 @@ static irqreturn_t ravb_interrupt(int irq, void *dev_id)
>  	struct net_device *ndev = dev_id;
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	const struct ravb_hw_info *info = priv->info;
> +	struct device *dev = &priv->pdev->dev;
>  	irqreturn_t result = IRQ_NONE;
>  	u32 iss;
>  
> +	pm_runtime_get_noresume(dev);
> +

   And here...

> +	if (unlikely(!pm_runtime_active(dev)))
> +		goto out_rpm_put;
> +
>  	spin_lock(&priv->lock);
>  	/* Get interrupt status */
>  	iss = ravb_read(ndev, ISS);
[...]
> @@ -1230,9 +1241,15 @@ static irqreturn_t ravb_multi_interrupt(int irq, void *dev_id)
>  {
>  	struct net_device *ndev = dev_id;
>  	struct ravb_private *priv = netdev_priv(ndev);
> +	struct device *dev = &priv->pdev->dev;
>  	irqreturn_t result = IRQ_NONE;
>  	u32 iss;
>  
> +	pm_runtime_get_noresume(dev);
> +

   Here too...

> +	if (unlikely(!pm_runtime_active(dev)))
> +		goto out_rpm_put;
> +
>  	spin_lock(&priv->lock);
>  	/* Get interrupt status */
>  	iss = ravb_read(ndev, ISS);
[...]
> @@ -1261,8 +1281,14 @@ static irqreturn_t ravb_dma_interrupt(int irq, void *dev_id, int q)
>  {
>  	struct net_device *ndev = dev_id;
>  	struct ravb_private *priv = netdev_priv(ndev);
> +	struct device *dev = &priv->pdev->dev;
>  	irqreturn_t result = IRQ_NONE;
>  
> +	pm_runtime_get_noresume(dev);
> +

   Here as well...

> +	if (unlikely(!pm_runtime_active(dev)))
> +		goto out_rpm_put;
> +
>  	spin_lock(&priv->lock);
>  
>  	/* Network control/Best effort queue RX/TX */
[...]
> @@ -2616,6 +2548,90 @@ static void ravb_parse_delay_mode(struct device_node *np, struct net_device *nde
>  	}
>  }
>  
> +static int ravb_setup_irq(struct ravb_private *priv, const char *irq_name,
> +			  const char *ch, int *irq, irq_handler_t handler)
> +{
> +	struct platform_device *pdev = priv->pdev;
> +	struct net_device *ndev = priv->ndev;
> +	struct device *dev = &pdev->dev;
> +	const char *dev_name;
> +	unsigned long flags;
> +	int error;
> +
> +	if (irq_name) {
> +		dev_name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch);
> +		if (!dev_name)
> +			return -ENOMEM;
> +
> +		*irq = platform_get_irq_byname(pdev, irq_name);
> +		flags = 0;
> +	} else {
> +		dev_name = ndev->name;
> +		*irq = platform_get_irq(pdev, 0);
> +		flags = IRQF_SHARED;

   Perhaps it's worth passing flags as a parameter here instead?

> +	}
> +	if (*irq < 0)
> +		return *irq;
> +
> +	error = devm_request_irq(dev, *irq, handler, flags, dev_name, ndev);
> +	if (error)
> +		netdev_err(ndev, "cannot request IRQ %s\n", dev_name);
> +
> +	return error;
> +}
[...]

MBR, Sergey