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

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

On 05.01.2024 22:57, Sergey Shtylyov wrote:
> On 1/5/24 11:23 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 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 <claudiu.beznea.uj@bp.renesas.com>
> [...]
>> 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