[net-next,v3,08/19] net: ravb: Move the IRQs get and request in the probe function

Commit Message

Claudiu Jan. 5, 2024, 8:23 a.m. UTC

From: Claudiu Beznea <[e-mail]>

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 specific 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 gets and requests are in a single place
there is no need to keep intermediary data (like ravb_rx_irqs[] and
ravb_tx_irqs[] arrays or IRQs in struct ravb_private).

This is a preparatory change to add runtime PM support for all IP variants.

Signed-off-by: Claudiu Beznea <[e-mail]>
---

Changes in v3:
- fixed typos in patch description
- detailed patch description
- reworked the code to have a single function doing IRQ get and
  request

Changes in v2:
- none; this patch is new


 drivers/net/ethernet/renesas/ravb.h      |   4 -
 drivers/net/ethernet/renesas/ravb_main.c | 258 ++++++++---------------
 2 files changed, 90 insertions(+), 172 deletions(-)

Comments

Sergey Shtylyov Jan. 5, 2024, 8:57 p.m. UTC | #1

On 1/5/24 11:23 AM, Claudiu wrote:

> From: Claudiu Beznea <[e-mail]>
> 
> 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 specific 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 gets and requests are in a single place
> there is no need to keep intermediary data (like ravb_rx_irqs[] and
> ravb_tx_irqs[] arrays or IRQs in struct ravb_private).
> 
> This is a preparatory change to add runtime PM support for all IP variants.
> 
> Signed-off-by: Claudiu Beznea <[e-mail]>
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index e0f8276cffed..e3506888cca6 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1089,10 +1089,6 @@ struct ravb_private {
>  	int msg_enable;
>  	int speed;
>  	int emac_irq;
> -	int erra_irq;
> -	int mgmta_irq;
> -	int rx_irqs[NUM_RX_QUEUE];
> -	int tx_irqs[NUM_TX_QUEUE];

   Good! :-)

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 4673cc2faec0..ac6488ffa29a 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -1727,85 +1717,21 @@ static const struct ethtool_ops ravb_ethtool_ops = {
>  	.set_wol		= ravb_set_wol,
>  };
>  
> -static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler,
> -				struct net_device *ndev, struct device *dev,
> -				const char *ch)
> -{
> -	char *name;
> -	int error;
> -
> -	name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch);

   Ugh! Should've fixed this outrage... :-/

[...]
> @@ -2616,6 +2509,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;
> +	}
> +	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", irq_name);

   What will be printed when irq_name is NULL? Shouldn't this be dev_name
instead?

> +
> +	return error;
> +}
> +
> +static int ravb_setup_irqs(struct ravb_private *priv)
> +{
> +	const struct ravb_hw_info *info = priv->info;
> +	struct net_device *ndev = priv->ndev;
> +	const char *irq_name, *emac_irq_name;
> +	int error, irq;
> +
> +	if (!info->multi_irqs)
> +		return ravb_setup_irq(priv, NULL, NULL, &ndev->irq, ravb_interrupt);
> +
> +	if (info->err_mgmt_irqs) {
> +		irq_name = "dia";
> +		emac_irq_name = "line3";
> +	} else {
> +		irq_name = "ch22";
> +		emac_irq_name = "ch24";
> +	}
> +
> +	error = ravb_setup_irq(priv, irq_name, "ch22:multi", &ndev->irq, ravb_multi_interrupt);
> +	if (error)
> +		return error;
> +
> +	error = ravb_setup_irq(priv, emac_irq_name, "ch24:emac", &priv->emac_irq,
> +			       ravb_emac_interrupt);
> +	if (error)
> +		return error;
> +
> +	if (info->err_mgmt_irqs) {
> +		error = ravb_setup_irq(priv, "err_a", "err_a", &irq, ravb_multi_interrupt);

   Hm, why pass 2 identical names?

> +		if (error)
> +			return error;
> +
> +		error = ravb_setup_irq(priv, "mgmt_a", "mgmt_a", &irq, ravb_multi_interrupt);

   Here as well?

> +		if (error)
> +			return error;
> +	}
> +
> +	error = ravb_setup_irq(priv, "ch0", "ch0:rx_be", &irq, ravb_be_interrupt);

   Hm, won't this result in "ch0:ch0:rx_be" as IRQ name?

> +	if (error)
> +		return error;
> +
> +	error = ravb_setup_irq(priv, "ch1", "ch1:rx_nc", &irq, ravb_nc_interrupt);

   Same question...

> +	if (error)
> +		return error;
> +
> +	error = ravb_setup_irq(priv, "ch18", "ch18:tx_be", &irq, ravb_be_interrupt);

   And here as well...

> +	if (error)
> +		return error;
> +
> +	return ravb_setup_irq(priv, "ch19", "ch19:tx_nc", &irq, ravb_nc_interrupt);

   Here too...

[...]

MBR, Sergey

Sergey Shtylyov Jan. 7, 2024, 6:24 p.m. UTC | #2

On 1/5/24 11:23 AM, Claudiu wrote:

> From: Claudiu Beznea <[e-mail]>
> 
> 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 specific 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 gets and requests are in a single place
> there is no need to keep intermediary data (like ravb_rx_irqs[] and
> ravb_tx_irqs[] arrays or IRQs in struct ravb_private).

   There's one thing that you probably didn't take into account: after
you call request_irq(), you should be able to handle your IRQ as it's
automatically unmasked, unless you pass IRQF_NO_AUTOEN to request_irq().
Your device may be held i reset or even powered off but if you pass IRQF_SHARED to request_irq() (you do in a single IRQ config), you must
be prepared to get your device's registers read (in order to ascertain
whether it's your IRQ or not). And you can't even pass IRQF_NO_AUTOEN
along with IRQF_SHARED, according to my reading of the IRQ code...

> This is a preparatory change to add runtime PM support for all IP variants.

  I don't readily see why this is necessary for the full-fledged RPM
support...

> Signed-off-by: Claudiu Beznea <[e-mail]>

   Unfortunately, I have to NAK this patch, at least in its current
form...

[...]

MBR, Sergey

Claudiu Jan. 8, 2024, 8:23 a.m. UTC | #3

On 05.01.2024 22:57, Sergey Shtylyov wrote:
> On 1/5/24 11:23 AM, Claudiu wrote:
> 
>> From: Claudiu Beznea <[e-mail]>
>>
>> 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 specific 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 gets and requests are in a single place
>> there is no need to keep intermediary data (like ravb_rx_irqs[] and
>> ravb_tx_irqs[] arrays or IRQs in struct ravb_private).
>>
>> This is a preparatory change to add runtime PM support for all IP variants.
>>
>> Signed-off-by: Claudiu Beznea <[e-mail]>
> [...]
>> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
>> index e0f8276cffed..e3506888cca6 100644
>> --- a/drivers/net/ethernet/renesas/ravb.h
>> +++ b/drivers/net/ethernet/renesas/ravb.h
>> @@ -1089,10 +1089,6 @@ struct ravb_private {
>>  	int msg_enable;
>>  	int speed;
>>  	int emac_irq;
>> -	int erra_irq;
>> -	int mgmta_irq;
>> -	int rx_irqs[NUM_RX_QUEUE];
>> -	int tx_irqs[NUM_TX_QUEUE];
> 
>    Good! :-)
> 
> [...]
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 4673cc2faec0..ac6488ffa29a 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
>> @@ -1727,85 +1717,21 @@ static const struct ethtool_ops ravb_ethtool_ops = {
>>  	.set_wol		= ravb_set_wol,
>>  };
>>  
>> -static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler,
>> -				struct net_device *ndev, struct device *dev,
>> -				const char *ch)
>> -{
>> -	char *name;
>> -	int error;
>> -
>> -	name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch);
> 
>    Ugh! Should've fixed this outrage... :-/
> 
> [...]
>> @@ -2616,6 +2509,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;
>> +	}
>> +	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", irq_name);
> 
>    What will be printed when irq_name is NULL? Shouldn't this be dev_name
> instead?

Indeed, should have been dev_name.

Maybe better would be to have irq_name and IRQ0 instead as the users of
this don't request IRQ from buf (where buf is sprintf(buf, "%s:%s",
ndev->name, ch)) but they request irq_name or IRQ0.

> 
>> +
>> +	return error;
>> +}
>> +
>> +static int ravb_setup_irqs(struct ravb_private *priv)
>> +{
>> +	const struct ravb_hw_info *info = priv->info;
>> +	struct net_device *ndev = priv->ndev;
>> +	const char *irq_name, *emac_irq_name;
>> +	int error, irq;
>> +
>> +	if (!info->multi_irqs)
>> +		return ravb_setup_irq(priv, NULL, NULL, &ndev->irq, ravb_interrupt);
>> +
>> +	if (info->err_mgmt_irqs) {
>> +		irq_name = "dia";
>> +		emac_irq_name = "line3";
>> +	} else {
>> +		irq_name = "ch22";
>> +		emac_irq_name = "ch24";
>> +	}
>> +
>> +	error = ravb_setup_irq(priv, irq_name, "ch22:multi", &ndev->irq, ravb_multi_interrupt);
>> +	if (error)
>> +		return error;
>> +
>> +	error = ravb_setup_irq(priv, emac_irq_name, "ch24:emac", &priv->emac_irq,
>> +			       ravb_emac_interrupt);
>> +	if (error)
>> +		return error;
>> +
>> +	if (info->err_mgmt_irqs) {
>> +		error = ravb_setup_irq(priv, "err_a", "err_a", &irq, ravb_multi_interrupt);
> 
>    Hm, why pass 2 identical names?

1st name is what is used by platform_get_irq_by_name(), 2nd name is used to
fill the name of the IRQ after it has been requested. Perviously the same
naming schema was used.

> 
>> +		if (error)
>> +			return error;
>> +
>> +		error = ravb_setup_irq(priv, "mgmt_a", "mgmt_a", &irq, ravb_multi_interrupt);
> 
>    Here as well?
> 
>> +		if (error)
>> +			return error;
>> +	}
>> +
>> +	error = ravb_setup_irq(priv, "ch0", "ch0:rx_be", &irq, ravb_be_interrupt);
> 
>    Hm, won't this result in "ch0:ch0:rx_be" as IRQ name?

No, first "ch0" is to call:
platform_get_irq_byname(pdev, "ch0");

"ch0:rx_be" is passed to
devm_kasprintf(..., "%s:%s", ndev->name, "ch0:rx_be");

and fill the name of IRQ after devm_request_irq(). Previously it was the same.

> 
>> +	if (error)
>> +		return error;
>> +
>> +	error = ravb_setup_irq(priv, "ch1", "ch1:rx_nc", &irq, ravb_nc_interrupt);
> 
>    Same question...
> 
>> +	if (error)
>> +		return error;
>> +
>> +	error = ravb_setup_irq(priv, "ch18", "ch18:tx_be", &irq, ravb_be_interrupt);
> 
>    And here as well...
> 
>> +	if (error)
>> +		return error;
>> +
>> +	return ravb_setup_irq(priv, "ch19", "ch19:tx_nc", &irq, ravb_nc_interrupt);
> 
>    Here too...
> 
> [...]
> 
> MBR, Sergey

Claudiu Jan. 8, 2024, 8:58 a.m. UTC | #4

On 07.01.2024 20:24, Sergey Shtylyov wrote:
> On 1/5/24 11:23 AM, Claudiu wrote:
> 
>> From: Claudiu Beznea <[e-mail]>
>>
>> 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 specific 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 gets and requests are in a single place
>> there is no need to keep intermediary data (like ravb_rx_irqs[] and
>> ravb_tx_irqs[] arrays or IRQs in struct ravb_private).
> 
>    There's one thing that you probably didn't take into account: after
> you call request_irq(), you should be able to handle your IRQ as it's
> automatically unmasked, unless you pass IRQF_NO_AUTOEN to request_irq().
> Your device may be held i reset or even powered off but if you pass IRQF_SHARED to request_irq() (you do in a single IRQ config), you must
> be prepared to get your device's registers read (in order to ascertain
> whether it's your IRQ or not). And you can't even pass IRQF_NO_AUTOEN
> along with IRQF_SHARED, according to my reading of the IRQ code...

Good point!

> 
>> This is a preparatory change to add runtime PM support for all IP variants.
> 
>   I don't readily see why this is necessary for the full-fledged RPM
> support...

I tried to speed up the ravb_open()/ravb_close() but missed the IRQF_SHARED
IRQ. As there is only one IRQ requested w/ IRQF_SHARED, are you OK with
still keeping the rest of IRQs handled as proposed by this patch?

> 
>> Signed-off-by: Claudiu Beznea <[e-mail]>
> 
>    Unfortunately, I have to NAK this patch, at least in its current
> form...
> 
> [...]
> 
> MBR, Sergey

Sergey Shtylyov Jan. 9, 2024, 8:47 p.m. UTC | #5

On 1/8/24 11:58 AM, claudiu beznea wrote:

[...]
>>> From: Claudiu Beznea <[e-mail]>
>>>
>>> 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 specific 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 gets and requests are in a single place
>>> there is no need to keep intermediary data (like ravb_rx_irqs[] and
>>> ravb_tx_irqs[] arrays or IRQs in struct ravb_private).
>>
>>    There's one thing that you probably didn't take into account: after
>> you call request_irq(), you should be able to handle your IRQ as it's
>> automatically unmasked, unless you pass IRQF_NO_AUTOEN to request_irq().
>> Your device may be held i reset or even powered off but if you pass IRQF_SHARED to request_irq() (you do in a single IRQ config), you must
>> be prepared to get your device's registers read (in order to ascertain

   And, at least on arm32, reading a powered off (or not clocked?) device's
register causes an imprecise external abort exception -- which results in a
kernel oops...

>> whether it's your IRQ or not). And you can't even pass IRQF_NO_AUTOEN
>> along with IRQF_SHARED, according to my reading of the IRQ code...
> 
> Good point!
> 
>>> This is a preparatory change to add runtime PM support for all IP variants.
>>
>>   I don't readily see why this is necessary for the full-fledged RPM
>> support...
> 
> I tried to speed up the ravb_open()/ravb_close() but missed the IRQF_SHARED

   I doubt that optimizing ravb_{open,close}() is worth pursuing, frankly...

> IRQ. As there is only one IRQ requested w/ IRQF_SHARED, are you OK with
> still keeping the rest of IRQs handled as proposed by this patch?

   I'm not, as this doesn't really seem necessary for your main goal.
It's not clear in what state U-Boot leaves EtherAVB...

[...]

MBR, Sergey

Claudiu Jan. 10, 2024, 11:55 a.m. UTC | #6

On 09.01.2024 22:47, Sergey Shtylyov wrote:
> On 1/8/24 11:58 AM, claudiu beznea wrote:
> 
> [...]
>>>> From: Claudiu Beznea <[e-mail]>
>>>>
>>>> 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 specific 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 gets and requests are in a single place
>>>> there is no need to keep intermediary data (like ravb_rx_irqs[] and
>>>> ravb_tx_irqs[] arrays or IRQs in struct ravb_private).
>>>
>>>    There's one thing that you probably didn't take into account: after
>>> you call request_irq(), you should be able to handle your IRQ as it's
>>> automatically unmasked, unless you pass IRQF_NO_AUTOEN to request_irq().
>>> Your device may be held i reset or even powered off but if you pass IRQF_SHARED to request_irq() (you do in a single IRQ config), you must
>>> be prepared to get your device's registers read (in order to ascertain
> 
>    And, at least on arm32, reading a powered off (or not clocked?) device's
> register causes an imprecise external abort exception -- which results in a
> kernel oops...
> 
>>> whether it's your IRQ or not). And you can't even pass IRQF_NO_AUTOEN
>>> along with IRQF_SHARED, according to my reading of the IRQ code...
>>
>> Good point!
>>
>>>> This is a preparatory change to add runtime PM support for all IP variants.
>>>
>>>   I don't readily see why this is necessary for the full-fledged RPM
>>> support...
>>
>> I tried to speed up the ravb_open()/ravb_close() but missed the IRQF_SHARED
> 
>    I doubt that optimizing ravb_{open,close}() is worth pursuing, frankly...
> 
>> IRQ. As there is only one IRQ requested w/ IRQF_SHARED, are you OK with
>> still keeping the rest of IRQs handled as proposed by this patch?
> 
>    I'm not, as this doesn't really seem necessary for your main goal.
> It's not clear in what state U-Boot leaves EtherAVB...

Ok. One other reason I did this is, as commit message states, to keep
resource parsing and allocation/freeing in probe/remove and hardware
settings in open/close.

Anyway, I'll revert all the changes IRQ related.

Thank you,
Claudiu Beznea

> 
> [...]
> 
> MBR, Sergey

Sergey Shtylyov Jan. 14, 2024, 6:07 p.m. UTC | #7

On 1/10/24 2:55 PM, claudiu beznea wrote:

[...]

>>>>> From: Claudiu Beznea <[e-mail]>
>>>>>
>>>>> 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 specific 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 gets and requests are in a single place
>>>>> there is no need to keep intermediary data (like ravb_rx_irqs[] and
>>>>> ravb_tx_irqs[] arrays or IRQs in struct ravb_private).
>>>>
>>>>    There's one thing that you probably didn't take into account: after
>>>> you call request_irq(), you should be able to handle your IRQ as it's
>>>> automatically unmasked, unless you pass IRQF_NO_AUTOEN to request_irq().
>>>> Your device may be held i reset or even powered off but if you pass
>>>> IRQF_SHARED to request_irq() (you do in a single IRQ config), you must
>>>> be prepared to get your device's registers read (in order to ascertain
>>
>>    And, at least on arm32, reading a powered off (or not clocked?) device's
>> register causes an imprecise external abort exception -- which results in a
>> kernel oops...
>>
>>>> whether it's your IRQ or not). And you can't even pass IRQF_NO_AUTOEN
>>>> along with IRQF_SHARED, according to my reading of the IRQ code...
>>>
>>> Good point!

   Iff we can come up with a robust check whether the device is powered on,
we can overcome even the IRQF_SHARED issue though...
   I'm seeing pm_runtime_active() API and wondering whether we can use it
from the IRQ context. Alternatively, we can add a is_opened flag, like
sh_eth.c does...

>>>>> This is a preparatory change to add runtime PM support for all IP variants.
>>>>
>>>>   I don't readily see why this is necessary for the full-fledged RPM
>>>> support...
>>>
>>> I tried to speed up the ravb_open()/ravb_close() but missed the IRQF_SHARED
>>
>>    I doubt that optimizing ravb_{open,close}() is worth pursuing, frankly...

   OTOH, we'll get a simpler/cleaner code if we do this...
   Previously, I was under an impression that it's common behavior of
the networking drivers to call request_irq() from their ndo_open() methods.
Apparently, it's not true anymore (probably with the introduction of the
managed device API) -- the newer drivers often call devm_request_irq()
from their probe() methods instead...

>>> IRQ. As there is only one IRQ requested w/ IRQF_SHARED, are you OK with
>>> still keeping the rest of IRQs handled as proposed by this patch?
>>
>>    I'm not, as this doesn't really seem necessary for your main goal.
>> It's not clear in what state U-Boot leaves EtherAVB...

   This still seems an issue though... My prior experience with the R-Car
MMC driver tells me that U-Boot may leave interrupts enabled... :-/

> Ok. One other reason I did this is, as commit message states, to keep
> resource parsing and allocation/freeing in probe/remove and hardware
> settings in open/close.
>  
> Anyway, I'll revert all the changes IRQ related.

   Now I've changed my mind -- let's retain your patch! It needs some work
though...

> Thank you,
> Claudiu Beznea

[...]

MBR, Sergey

Claudiu Jan. 15, 2024, 7:10 a.m. UTC | #8

On 14.01.2024 20:07, Sergey Shtylyov wrote:
> On 1/10/24 2:55 PM, claudiu beznea wrote:
> 
> [...]
> 
>>>>>> From: Claudiu Beznea <[e-mail]>
>>>>>>
>>>>>> 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 specific 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 gets and requests are in a single place
>>>>>> there is no need to keep intermediary data (like ravb_rx_irqs[] and
>>>>>> ravb_tx_irqs[] arrays or IRQs in struct ravb_private).
>>>>>
>>>>>    There's one thing that you probably didn't take into account: after
>>>>> you call request_irq(), you should be able to handle your IRQ as it's
>>>>> automatically unmasked, unless you pass IRQF_NO_AUTOEN to request_irq().
>>>>> Your device may be held i reset or even powered off but if you pass
>>>>> IRQF_SHARED to request_irq() (you do in a single IRQ config), you must
>>>>> be prepared to get your device's registers read (in order to ascertain
>>>
>>>    And, at least on arm32, reading a powered off (or not clocked?) device's
>>> register causes an imprecise external abort exception -- which results in a
>>> kernel oops...
>>>
>>>>> whether it's your IRQ or not). And you can't even pass IRQF_NO_AUTOEN
>>>>> along with IRQF_SHARED, according to my reading of the IRQ code...
>>>>
>>>> Good point!
> 
>    Iff we can come up with a robust check whether the device is powered on,
> we can overcome even the IRQF_SHARED issue though...
>    I'm seeing pm_runtime_active() API and wondering whether we can use it
> from the IRQ context. Alternatively, we can add a is_opened flag, like
> sh_eth.c does...

The is_open flag should deal with this, too, AFAICT at the moment, and
should also cover your concerns about U-Boot.

Thank you,
Claudiu Beznea

> 
>>>>>> This is a preparatory change to add runtime PM support for all IP variants.
>>>>>
>>>>>   I don't readily see why this is necessary for the full-fledged RPM
>>>>> support...
>>>>
>>>> I tried to speed up the ravb_open()/ravb_close() but missed the IRQF_SHARED
>>>
>>>    I doubt that optimizing ravb_{open,close}() is worth pursuing, frankly...
> 
>    OTOH, we'll get a simpler/cleaner code if we do this...
>    Previously, I was under an impression that it's common behavior of
> the networking drivers to call request_irq() from their ndo_open() methods.
> Apparently, it's not true anymore (probably with the introduction of the
> managed device API) -- the newer drivers often call devm_request_irq()
> from their probe() methods instead...
> 
>>>> IRQ. As there is only one IRQ requested w/ IRQF_SHARED, are you OK with
>>>> still keeping the rest of IRQs handled as proposed by this patch?
>>>
>>>    I'm not, as this doesn't really seem necessary for your main goal.
>>> It's not clear in what state U-Boot leaves EtherAVB...
> 
>    This still seems an issue though... My prior experience with the R-Car
> MMC driver tells me that U-Boot may leave interrupts enabled... :-/
> 
>> Ok. One other reason I did this is, as commit message states, to keep
>> resource parsing and allocation/freeing in probe/remove and hardware
>> settings in open/close.
>>  
>> Anyway, I'll revert all the changes IRQ related.
> 
>    Now I've changed my mind -- let's retain your patch! It needs some work
> though...
> 
>> Thank you,
>> Claudiu Beznea
> 
> [...]
> 
> MBR, Sergey

Patch

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index e0f8276cffed..e3506888cca6 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1089,10 +1089,6 @@  struct ravb_private {
 	int msg_enable;
 	int speed;
 	int emac_irq;
-	int erra_irq;
-	int mgmta_irq;
-	int rx_irqs[NUM_RX_QUEUE];
-	int tx_irqs[NUM_TX_QUEUE];
 
 	unsigned no_avb_link:1;
 	unsigned avb_link_active_low:1;
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 4673cc2faec0..ac6488ffa29a 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -38,16 +38,6 @@ 
 		 NETIF_MSG_RX_ERR | \
 		 NETIF_MSG_TX_ERR)
 
-static const char *ravb_rx_irqs[NUM_RX_QUEUE] = {
-	"ch0", /* RAVB_BE */
-	"ch1", /* RAVB_NC */
-};
-
-static const char *ravb_tx_irqs[NUM_TX_QUEUE] = {
-	"ch18", /* RAVB_BE */
-	"ch19", /* RAVB_NC */
-};
-
 void ravb_modify(struct net_device *ndev, enum ravb_reg reg, u32 clear,
 		 u32 set)
 {
@@ -1727,85 +1717,21 @@  static const struct ethtool_ops ravb_ethtool_ops = {
 	.set_wol		= ravb_set_wol,
 };
 
-static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler,
-				struct net_device *ndev, struct device *dev,
-				const char *ch)
-{
-	char *name;
-	int error;
-
-	name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch);
-	if (!name)
-		return -ENOMEM;
-	error = request_irq(irq, handler, 0, name, ndev);
-	if (error)
-		netdev_err(ndev, "cannot request IRQ %s\n", name);
-
-	return error;
-}
-
 /* Network device open function for Ethernet AVB */
 static int ravb_open(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
-	struct platform_device *pdev = priv->pdev;
-	struct device *dev = &pdev->dev;
 	int error;
 
 	napi_enable(&priv->napi[RAVB_BE]);
 	if (info->nc_queues)
 		napi_enable(&priv->napi[RAVB_NC]);
 
-	if (!info->multi_irqs) {
-		error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED,
-				    ndev->name, ndev);
-		if (error) {
-			netdev_err(ndev, "cannot request IRQ\n");
-			goto out_napi_off;
-		}
-	} else {
-		error = ravb_hook_irq(ndev->irq, ravb_multi_interrupt, ndev,
-				      dev, "ch22:multi");
-		if (error)
-			goto out_napi_off;
-		error = ravb_hook_irq(priv->emac_irq, ravb_emac_interrupt, ndev,
-				      dev, "ch24:emac");
-		if (error)
-			goto out_free_irq;
-		error = ravb_hook_irq(priv->rx_irqs[RAVB_BE], ravb_be_interrupt,
-				      ndev, dev, "ch0:rx_be");
-		if (error)
-			goto out_free_irq_emac;
-		error = ravb_hook_irq(priv->tx_irqs[RAVB_BE], ravb_be_interrupt,
-				      ndev, dev, "ch18:tx_be");
-		if (error)
-			goto out_free_irq_be_rx;
-		error = ravb_hook_irq(priv->rx_irqs[RAVB_NC], ravb_nc_interrupt,
-				      ndev, dev, "ch1:rx_nc");
-		if (error)
-			goto out_free_irq_be_tx;
-		error = ravb_hook_irq(priv->tx_irqs[RAVB_NC], ravb_nc_interrupt,
-				      ndev, dev, "ch19:tx_nc");
-		if (error)
-			goto out_free_irq_nc_rx;
-
-		if (info->err_mgmt_irqs) {
-			error = ravb_hook_irq(priv->erra_irq, ravb_multi_interrupt,
-					      ndev, dev, "err_a");
-			if (error)
-				goto out_free_irq_nc_tx;
-			error = ravb_hook_irq(priv->mgmta_irq, ravb_multi_interrupt,
-					      ndev, dev, "mgmt_a");
-			if (error)
-				goto out_free_irq_erra;
-		}
-	}
-
 	/* Device init */
 	error = ravb_dmac_init(ndev);
 	if (error)
-		goto out_free_irq_mgmta;
+		goto out_napi_off;
 	ravb_emac_init(ndev);
 
 	/* Initialise PTP Clock driver */
@@ -1826,26 +1752,6 @@  static int ravb_open(struct net_device *ndev)
 	if (info->gptp)
 		ravb_ptp_stop(ndev);
 	ravb_stop_dma(ndev);
-out_free_irq_mgmta:
-	if (!info->multi_irqs)
-		goto out_free_irq;
-	if (info->err_mgmt_irqs)
-		free_irq(priv->mgmta_irq, ndev);
-out_free_irq_erra:
-	if (info->err_mgmt_irqs)
-		free_irq(priv->erra_irq, ndev);
-out_free_irq_nc_tx:
-	free_irq(priv->tx_irqs[RAVB_NC], ndev);
-out_free_irq_nc_rx:
-	free_irq(priv->rx_irqs[RAVB_NC], ndev);
-out_free_irq_be_tx:
-	free_irq(priv->tx_irqs[RAVB_BE], ndev);
-out_free_irq_be_rx:
-	free_irq(priv->rx_irqs[RAVB_BE], ndev);
-out_free_irq_emac:
-	free_irq(priv->emac_irq, ndev);
-out_free_irq:
-	free_irq(ndev->irq, ndev);
 out_napi_off:
 	if (info->nc_queues)
 		napi_disable(&priv->napi[RAVB_NC]);
@@ -2180,19 +2086,6 @@  static int ravb_close(struct net_device *ndev)
 
 	cancel_work_sync(&priv->work);
 
-	if (info->multi_irqs) {
-		free_irq(priv->tx_irqs[RAVB_NC], ndev);
-		free_irq(priv->rx_irqs[RAVB_NC], ndev);
-		free_irq(priv->tx_irqs[RAVB_BE], ndev);
-		free_irq(priv->rx_irqs[RAVB_BE], ndev);
-		free_irq(priv->emac_irq, ndev);
-		if (info->err_mgmt_irqs) {
-			free_irq(priv->erra_irq, ndev);
-			free_irq(priv->mgmta_irq, ndev);
-		}
-	}
-	free_irq(ndev->irq, ndev);
-
 	if (info->nc_queues)
 		napi_disable(&priv->napi[RAVB_NC]);
 	napi_disable(&priv->napi[RAVB_BE]);
@@ -2616,6 +2509,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;
+	}
+	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", irq_name);
+
+	return error;
+}
+
+static int ravb_setup_irqs(struct ravb_private *priv)
+{
+	const struct ravb_hw_info *info = priv->info;
+	struct net_device *ndev = priv->ndev;
+	const char *irq_name, *emac_irq_name;
+	int error, irq;
+
+	if (!info->multi_irqs)
+		return ravb_setup_irq(priv, NULL, NULL, &ndev->irq, ravb_interrupt);
+
+	if (info->err_mgmt_irqs) {
+		irq_name = "dia";
+		emac_irq_name = "line3";
+	} else {
+		irq_name = "ch22";
+		emac_irq_name = "ch24";
+	}
+
+	error = ravb_setup_irq(priv, irq_name, "ch22:multi", &ndev->irq, ravb_multi_interrupt);
+	if (error)
+		return error;
+
+	error = ravb_setup_irq(priv, emac_irq_name, "ch24:emac", &priv->emac_irq,
+			       ravb_emac_interrupt);
+	if (error)
+		return error;
+
+	if (info->err_mgmt_irqs) {
+		error = ravb_setup_irq(priv, "err_a", "err_a", &irq, ravb_multi_interrupt);
+		if (error)
+			return error;
+
+		error = ravb_setup_irq(priv, "mgmt_a", "mgmt_a", &irq, ravb_multi_interrupt);
+		if (error)
+			return error;
+	}
+
+	error = ravb_setup_irq(priv, "ch0", "ch0:rx_be", &irq, ravb_be_interrupt);
+	if (error)
+		return error;
+
+	error = ravb_setup_irq(priv, "ch1", "ch1:rx_nc", &irq, ravb_nc_interrupt);
+	if (error)
+		return error;
+
+	error = ravb_setup_irq(priv, "ch18", "ch18:tx_be", &irq, ravb_be_interrupt);
+	if (error)
+		return error;
+
+	return ravb_setup_irq(priv, "ch19", "ch19:tx_nc", &irq, ravb_nc_interrupt);
+}
+
 static void ravb_set_delay_mode(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -2635,9 +2612,8 @@  static int ravb_probe(struct platform_device *pdev)
 	struct reset_control *rstc;
 	struct ravb_private *priv;
 	struct net_device *ndev;
-	int error, irq, q;
 	struct resource *res;
-	int i;
+	int error, q;
 
 	if (!np) {
 		dev_err(&pdev->dev,
@@ -2664,20 +2640,6 @@  static int ravb_probe(struct platform_device *pdev)
 	if (error)
 		goto out_free_netdev;
 
-	if (info->multi_irqs) {
-		if (info->err_mgmt_irqs)
-			irq = platform_get_irq_byname(pdev, "dia");
-		else
-			irq = platform_get_irq_byname(pdev, "ch22");
-	} else {
-		irq = platform_get_irq(pdev, 0);
-	}
-	if (irq < 0) {
-		error = irq;
-		goto out_reset_assert;
-	}
-	ndev->irq = irq;
-
 	SET_NETDEV_DEV(ndev, &pdev->dev);
 
 	priv = netdev_priv(ndev);
@@ -2692,6 +2654,10 @@  static int ravb_probe(struct platform_device *pdev)
 		priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
 	}
 
+	error = ravb_setup_irqs(priv);
+	if (error)
+		goto out_reset_assert;
+
 	priv->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		error = PTR_ERR(priv->clk);
@@ -2739,50 +2705,6 @@  static int ravb_probe(struct platform_device *pdev)
 	priv->avb_link_active_low =
 		of_property_read_bool(np, "renesas,ether-link-active-low");
 
-	if (info->multi_irqs) {
-		if (info->err_mgmt_irqs)
-			irq = platform_get_irq_byname(pdev, "line3");
-		else
-			irq = platform_get_irq_byname(pdev, "ch24");
-		if (irq < 0) {
-			error = irq;
-			goto out_rpm_put;
-		}
-		priv->emac_irq = irq;
-		for (i = 0; i < NUM_RX_QUEUE; i++) {
-			irq = platform_get_irq_byname(pdev, ravb_rx_irqs[i]);
-			if (irq < 0) {
-				error = irq;
-				goto out_rpm_put;
-			}
-			priv->rx_irqs[i] = irq;
-		}
-		for (i = 0; i < NUM_TX_QUEUE; i++) {
-			irq = platform_get_irq_byname(pdev, ravb_tx_irqs[i]);
-			if (irq < 0) {
-				error = irq;
-				goto out_rpm_put;
-			}
-			priv->tx_irqs[i] = irq;
-		}
-
-		if (info->err_mgmt_irqs) {
-			irq = platform_get_irq_byname(pdev, "err_a");
-			if (irq < 0) {
-				error = irq;
-				goto out_rpm_put;
-			}
-			priv->erra_irq = irq;
-
-			irq = platform_get_irq_byname(pdev, "mgmt_a");
-			if (irq < 0) {
-				error = irq;
-				goto out_rpm_put;
-			}
-			priv->mgmta_irq = irq;
-		}
-	}
-
 	ndev->max_mtu = info->rx_max_buf_size - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
 	ndev->min_mtu = ETH_MIN_MTU;