Re: [PATCH net-next v5 08/15] net: ravb: Move the IRQs getting/requesting in the probe() method

[claudiu beznea <claudiu.beznea@tuxon.dev>]

On 31.01.2024 21:51, Sergey Shtylyov wrote:
>    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! =)

It seems I messed this up.

> 
> 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? :-)

Yes, it should be safe.

> 
>> 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...

That's a personal taste... more like to emphasize that this is PM runtime
"protected"... Same for the rest of occurrences you signaled below.

> 
>> +	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?

I don't see it like this. We need this flag for a single call of
ravb_setup_irq(), we can determine for which call we need to set this flag
so I think it is redundant to have an extra argument for it.

> 
>> +	}
>> +	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