Re: [PATCH] PCI: aspeed: Switch to irq_domain_create_linear()

[Jiri Slaby <jirislaby@kernel.org>]

On 08. 07. 26, 12:04, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The commit message claims to use `of_fwnode_handle()`, but the code actually uses `dev_fwnode()`.
> --
> 
> --- Patch [1]: [PATCH] PCI: aspeed: Switch to irq_domain_create_linear() ---
> commit aee46b37ad2da511a238368770cbd10115b36171
> Author: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> 
> PCI: aspeed: Switch to irq_domain_create_linear()
> 
> This commit updates the driver to use the modern irq_domain_create_linear()
> function instead of the obsolete irq_domain_add_linear(). It adjusts the
> first parameter to pass a fwnode_handle rather than a device_node.
> 
>> irq_domain_add_linear() is going away as being obsolete now. Switch to
>> the preferred irq_domain_create_linear(). That differs in the first
>> parameter: It takes more generic struct fwnode_handle instead of struct
>> device_node. Therefore, of_fwnode_handle() is added around the
>> parameter.
>>
>> Note some of the users can likely use dev->fwnode directly instead of
>> indirect of_fwnode_handle(dev->of_node). But dev->fwnode is not
>> guaranteed to be set for all, so this has to be investigated on case to
>> case basis (by people who can actually test with the HW).
> 
>> diff --git a/drivers/pci/controller/pcie-aspeed.c b/drivers/pci/controller/pcie-aspeed.c
>> index 9aa9e14c61484..74b03a55643fa 100644
>> --- a/drivers/pci/controller/pcie-aspeed.c
>> +++ b/drivers/pci/controller/pcie-aspeed.c
>> @@ -725,10 +725,10 @@ static int aspeed_pcie_init_irq_domain(struct aspeed_pcie *pcie)
>>   {
>>   	int ret;
>>   
>> -	pcie->intx_domain = irq_domain_add_linear(pcie->dev->of_node,
>> -						  PCI_NUM_INTX,
>> -						  &aspeed_intx_domain_ops,
>> -						  pcie);
>> +	pcie->intx_domain = irq_domain_create_linear(dev_fwnode(pcie->dev),
>> +						     PCI_NUM_INTX,
>> +						     &aspeed_intx_domain_ops,
>> +						     pcie);
> 
> [Severity: Medium]
> Does this code change contradict the commit message?
> 
> The commit message explicitly mentions adding of_fwnode_handle() around the
> parameter and warns against using dev->fwnode directly since it is not
> guaranteed to be set.

Sashiko is right, I used a template for more patches...

> The patch uses dev_fwnode(pcie->dev) here, which fetches dev->fwnode, rather
> than using of_fwnode_handle(pcie->dev->of_node) as described. Should the
> implementation be updated to use of_fwnode_handle(), or does the commit
> message need to be revised to match this dev_fwnode() approach?

In this very case, dev_fwnode(pcie->dev) is used in 
aspeed_pcie_msi_init(), called few lines below, so this is all right. 
The commit log is obsolete in this case though. Do you want me to resubmit?

thanks,
-- 
js
suse labs