[PATCH] drivers: block: save return value of pci_find_capability() in u8

[PATCH] drivers: block: save return value of pci_find_capability() in u8

[Puranjay Mohan]

Callers of pci_find_capability should save the return value in u8.
change type of variables from int to u8 to match the specification.

Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
---
 drivers/block/mtip32xx/mtip32xx.c | 2 +-
 drivers/block/skd_main.c          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
index 153e2cdecb4d..da57d37c6d20 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -3936,7 +3936,7 @@ static DEFINE_HANDLER(7);
 
 static void mtip_disable_link_opts(struct driver_data *dd, struct pci_dev *pdev)
 {
-	int pos;
+	u8 pos;
 	unsigned short pcie_dev_ctrl;
 
 	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
index a962b4551bed..16d59569129b 100644
--- a/drivers/block/skd_main.c
+++ b/drivers/block/skd_main.c
@@ -3136,7 +3136,7 @@ MODULE_DEVICE_TABLE(pci, skd_pci_tbl);
 
 static char *skd_pci_info(struct skd_device *skdev, char *str)
 {
-	int pcie_reg;
+	u8 pcie_reg;
 
 	strcpy(str, "PCIe (");
 	pcie_reg = pci_find_capability(skdev->pdev, PCI_CAP_ID_EXP);
-- 
2.27.0

Re: [PATCH] drivers: block: save return value of pci_find_capability() in u8

[Chaitanya Kulkarni]

On 12/6/20 11:45, Puranjay Mohan wrote:
> Callers of pci_find_capability should save the return value in u8.
> change type of variables from int to u8 to match the specification.

I did not understand this, pci_find_capability() does not return u8. 

what is it that we are achieving by changing the variable type ?

This patch will probably also generate type mismatch warning with

certain static analyzers.

> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> ---
>  drivers/block/mtip32xx/mtip32xx.c | 2 +-
>  drivers/block/skd_main.c          | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
> index 153e2cdecb4d..da57d37c6d20 100644
> --- a/drivers/block/mtip32xx/mtip32xx.c
> +++ b/drivers/block/mtip32xx/mtip32xx.c
> @@ -3936,7 +3936,7 @@ static DEFINE_HANDLER(7);
>  
>  static void mtip_disable_link_opts(struct driver_data *dd, struct pci_dev *pdev)
>  {
> -	int pos;
> +	u8 pos;
>  	unsigned short pcie_dev_ctrl;
>  
>  	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
> diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
> index a962b4551bed..16d59569129b 100644
> --- a/drivers/block/skd_main.c
> +++ b/drivers/block/skd_main.c
> @@ -3136,7 +3136,7 @@ MODULE_DEVICE_TABLE(pci, skd_pci_tbl);
>  
>  static char *skd_pci_info(struct skd_device *skdev, char *str)
>  {
> -	int pcie_reg;
> +	u8 pcie_reg;
>  
>  	strcpy(str, "PCIe (");
>  	pcie_reg = pci_find_capability(skdev->pdev, PCI_CAP_ID_EXP);

Re: [PATCH] drivers: block: save return value of pci_find_capability() in u8

[Bjorn Helgaas]

On Sun, Dec 06, 2020 at 11:08:14PM +0000, Chaitanya Kulkarni wrote:
> On 12/6/20 11:45, Puranjay Mohan wrote:
> > Callers of pci_find_capability should save the return value in u8.
> > change type of variables from int to u8 to match the specification.
> 
> I did not understand this, pci_find_capability() does not return u8. 
> 
> what is it that we are achieving by changing the variable type ?
> 
> This patch will probably also generate type mismatch warning with
> 
> certain static analyzers.

There's a patch pending via the PCI tree to change the return type to
u8.  We can do one of:

  - Ignore this.  It only changes something on the stack, so no real
    space saving and there's no problem assigning the u8 return value
    to the "int".

  - The maintainer could ack it and I could merge it via the PCI tree
    so it happens in the correct order (after the interface change).

  - The PCI core interface change will be merged for v5.11, so we
    could hold this until v5.12.

I don't really have a preference.  The only place there would really
be a benefit would be if we store the return value in a struct, where
we could potentially save three bytes.

Bjorn

> > Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> > ---
> >  drivers/block/mtip32xx/mtip32xx.c | 2 +-
> >  drivers/block/skd_main.c          | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
> > index 153e2cdecb4d..da57d37c6d20 100644
> > --- a/drivers/block/mtip32xx/mtip32xx.c
> > +++ b/drivers/block/mtip32xx/mtip32xx.c
> > @@ -3936,7 +3936,7 @@ static DEFINE_HANDLER(7);
> >  
> >  static void mtip_disable_link_opts(struct driver_data *dd, struct pci_dev *pdev)
> >  {
> > -	int pos;
> > +	u8 pos;
> >  	unsigned short pcie_dev_ctrl;
> >  
> >  	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
> > diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
> > index a962b4551bed..16d59569129b 100644
> > --- a/drivers/block/skd_main.c
> > +++ b/drivers/block/skd_main.c
> > @@ -3136,7 +3136,7 @@ MODULE_DEVICE_TABLE(pci, skd_pci_tbl);
> >  
> >  static char *skd_pci_info(struct skd_device *skdev, char *str)
> >  {
> > -	int pcie_reg;
> > +	u8 pcie_reg;
> >  
> >  	strcpy(str, "PCIe (");
> >  	pcie_reg = pci_find_capability(skdev->pdev, PCI_CAP_ID_EXP);
> 
> 

Re: [PATCH] drivers: block: save return value of pci_find_capability() in u8

[Damien Le Moal]

On 2020/12/07 10:26, Bjorn Helgaas wrote:
> On Sun, Dec 06, 2020 at 11:08:14PM +0000, Chaitanya Kulkarni wrote:
>> On 12/6/20 11:45, Puranjay Mohan wrote:
>>> Callers of pci_find_capability should save the return value in u8.
>>> change type of variables from int to u8 to match the specification.
>>
>> I did not understand this, pci_find_capability() does not return u8. 
>>
>> what is it that we are achieving by changing the variable type ?
>>
>> This patch will probably also generate type mismatch warning with
>>
>> certain static analyzers.
> 
> There's a patch pending via the PCI tree to change the return type to
> u8.  We can do one of:
> 
>   - Ignore this.  It only changes something on the stack, so no real
>     space saving and there's no problem assigning the u8 return value
>     to the "int".
> 
>   - The maintainer could ack it and I could merge it via the PCI tree
>     so it happens in the correct order (after the interface change).

That works for me. But this driver changes generally go through Jens block tree.

Jens,

Is this OK with you if Bjorn takes the patch through the PCI tree ?

> 
>   - The PCI core interface change will be merged for v5.11, so we
>     could hold this until v5.12.
> 
> I don't really have a preference.  The only place there would really
> be a benefit would be if we store the return value in a struct, where
> we could potentially save three bytes.
> 
> Bjorn
> 
>>> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
>>> ---
>>>  drivers/block/mtip32xx/mtip32xx.c | 2 +-
>>>  drivers/block/skd_main.c          | 2 +-
>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
>>> index 153e2cdecb4d..da57d37c6d20 100644
>>> --- a/drivers/block/mtip32xx/mtip32xx.c
>>> +++ b/drivers/block/mtip32xx/mtip32xx.c
>>> @@ -3936,7 +3936,7 @@ static DEFINE_HANDLER(7);
>>>  
>>>  static void mtip_disable_link_opts(struct driver_data *dd, struct pci_dev *pdev)
>>>  {
>>> -	int pos;
>>> +	u8 pos;
>>>  	unsigned short pcie_dev_ctrl;
>>>  
>>>  	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
>>> diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
>>> index a962b4551bed..16d59569129b 100644
>>> --- a/drivers/block/skd_main.c
>>> +++ b/drivers/block/skd_main.c
>>> @@ -3136,7 +3136,7 @@ MODULE_DEVICE_TABLE(pci, skd_pci_tbl);
>>>  
>>>  static char *skd_pci_info(struct skd_device *skdev, char *str)
>>>  {
>>> -	int pcie_reg;
>>> +	u8 pcie_reg;
>>>  
>>>  	strcpy(str, "PCIe (");
>>>  	pcie_reg = pci_find_capability(skdev->pdev, PCI_CAP_ID_EXP);
>>
>>
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] drivers: block: save return value of pci_find_capability() in u8

[Jens Axboe]

On 12/6/20 6:30 PM, Damien Le Moal wrote:
> On 2020/12/07 10:26, Bjorn Helgaas wrote:
>> On Sun, Dec 06, 2020 at 11:08:14PM +0000, Chaitanya Kulkarni wrote:
>>> On 12/6/20 11:45, Puranjay Mohan wrote:
>>>> Callers of pci_find_capability should save the return value in u8.
>>>> change type of variables from int to u8 to match the specification.
>>>
>>> I did not understand this, pci_find_capability() does not return u8. 
>>>
>>> what is it that we are achieving by changing the variable type ?
>>>
>>> This patch will probably also generate type mismatch warning with
>>>
>>> certain static analyzers.
>>
>> There's a patch pending via the PCI tree to change the return type to
>> u8.  We can do one of:
>>
>>   - Ignore this.  It only changes something on the stack, so no real
>>     space saving and there's no problem assigning the u8 return value
>>     to the "int".
>>
>>   - The maintainer could ack it and I could merge it via the PCI tree
>>     so it happens in the correct order (after the interface change).
> 
> That works for me. But this driver changes generally go through Jens block tree.
> 
> Jens,
> 
> Is this OK with you if Bjorn takes the patch through the PCI tree ?

Yep that's fine, if that makes it easier to handle.

-- 
Jens Axboe

Re: [PATCH] drivers: block: save return value of pci_find_capability() in u8

[Chaitanya Kulkarni]

Bjorn,
On 12/6/20 17:26, Bjorn Helgaas wrote:
> There's a patch pending via the PCI tree to change the return type to
> u8.  We can do one of:
I did not know about the pending patch, if that is going to change the
return

type then this patch makes sense.

>
>   - Ignore this.  It only changes something on the stack, so no real
>     space saving and there's no problem assigning the u8 return value
>     to the "int".
>   - The maintainer could ack it and I could merge it via the PCI tree
>     so it happens in the correct order (after the interface change).
If we want it in 5.11 then I think PCI tree is a right way go about it.
>   - The PCI core interface change will be merged for v5.11, so we
>     could hold this until v5.12.
> I don't really have a preference.  The only place there would really
> be a benefit would be if we store the return value in a struct, where
> we could potentially save three bytes.
Totally agree.
> Bjorn

Whoever is going to apply please add :-

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Re: [PATCH] drivers: block: save return value of pci_find_capability() in u8

[Ben Dooks]

On 07/12/2020 01:26, Bjorn Helgaas wrote:
> On Sun, Dec 06, 2020 at 11:08:14PM +0000, Chaitanya Kulkarni wrote:
>> On 12/6/20 11:45, Puranjay Mohan wrote:
>>> Callers of pci_find_capability should save the return value in u8.
>>> change type of variables from int to u8 to match the specification.the com
>>
>> I did not understand this, pci_find_capability() does not return u8.
>>
>> what is it that we are achieving by changing the variable type ?
>>
>> This patch will probably also generate type mismatch warning with
>>
>> certain static analyzers.
> 
> There's a patch pending via the PCI tree to change the return type to
> u8.  We can do one of:
> 
>    - Ignore this.  It only changes something on the stack, so no real
>      space saving and there's no problem assigning the u8 return value
>      to the "int".

I seem to remember some compilers would emit a sequence to constrain the
result to a valid char, but that looks like it is fixed in gcc-9 if the
input was also u8

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [PATCH] drivers: block: save return value of pci_find_capability() in u8

[Damien Le Moal]

On 2020/12/07 4:43, Puranjay Mohan wrote:
> Callers of pci_find_capability should save the return value in u8.
> change type of variables from int to u8 to match the specification.
> 
> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> ---
>  drivers/block/mtip32xx/mtip32xx.c | 2 +-
>  drivers/block/skd_main.c          | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
> index 153e2cdecb4d..da57d37c6d20 100644
> --- a/drivers/block/mtip32xx/mtip32xx.c
> +++ b/drivers/block/mtip32xx/mtip32xx.c
> @@ -3936,7 +3936,7 @@ static DEFINE_HANDLER(7);
>  
>  static void mtip_disable_link_opts(struct driver_data *dd, struct pci_dev *pdev)
>  {
> -	int pos;
> +	u8 pos;
>  	unsigned short pcie_dev_ctrl;
>  
>  	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
> diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
> index a962b4551bed..16d59569129b 100644
> --- a/drivers/block/skd_main.c
> +++ b/drivers/block/skd_main.c
> @@ -3136,7 +3136,7 @@ MODULE_DEVICE_TABLE(pci, skd_pci_tbl);
>  
>  static char *skd_pci_info(struct skd_device *skdev, char *str)
>  {
> -	int pcie_reg;
> +	u8 pcie_reg;
>  
>  	strcpy(str, "PCIe (");
>  	pcie_reg = pci_find_capability(skdev->pdev, PCI_CAP_ID_EXP);
> 

Acked-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] drivers: block: save return value of pci_find_capability() in u8

[Christoph Hellwig]

Can we take a step back?  I think in general drivers should not bother
with pci_find_capability.  Both mtip32xx and skd want to find out if
the devices are PCIe devices, skd then just prints the link speed for
which we have much better helpers, mtip32xx than messes with DEVCTL
which seems actively dangerous to me.

Re: [PATCH] drivers: block: save return value of pci_find_capability() in u8

[Bjorn Helgaas]

On Mon, Dec 07, 2020 at 02:52:58PM +0000, Christoph Hellwig wrote:
> Can we take a step back?  I think in general drivers should not bother
> with pci_find_capability.  Both mtip32xx and skd want to find out if
> the devices are PCIe devices, skd then just prints the link speed for
> which we have much better helpers, mtip32xx than messes with DEVCTL
> which seems actively dangerous to me.

Yes, that's a much better idea.  Plus it would be a much more
interesting mentorship project.

Re: [PATCH] drivers: block: save return value of pci_find_capability() in u8

[Puranjay Mohan]

On Mon, Dec 7, 2020 at 8:23 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> Can we take a step back?  I think in general drivers should not bother
> with pci_find_capability.  Both mtip32xx and skd want to find out if
> the devices are PCIe devices, skd then just prints the link speed for
> which we have much better helpers, mtip32xx than messes with DEVCTL
> which seems actively dangerous to me.

The skd driver uses pci_find_capability(skdev->pdev, PCI_CAP_ID_EXP);
to check if the device is PCIe, it can use pci_is_pcie() to do that.
After that it uses pci_read_config_word(skdev->pdev, pcie_reg,
&pcie_lstat); to read the Link Status Register, I think
it should use pcie_capability_read_word(skdev->pdev, PCI_EXP_LNKSTA,
&pcie_lstat);

Please let me know if the above changes are correct so I may send a patch.
-- 
Thanks and Regards

Yours Truly,

Puranjay Mohan

Re: [PATCH] drivers: block: save return value of pci_find_capability() in u8

[Keith Busch]

On Mon, Dec 07, 2020 at 11:23:03PM +0530, Puranjay Mohan wrote:
> On Mon, Dec 7, 2020 at 8:23 PM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > Can we take a step back?  I think in general drivers should not bother
> > with pci_find_capability.  Both mtip32xx and skd want to find out if
> > the devices are PCIe devices, skd then just prints the link speed for
> > which we have much better helpers, mtip32xx than messes with DEVCTL
> > which seems actively dangerous to me.
> 
> The skd driver uses pci_find_capability(skdev->pdev, PCI_CAP_ID_EXP);
> to check if the device is PCIe, it can use pci_is_pcie() to do that.
> After that it uses pci_read_config_word(skdev->pdev, pcie_reg,
> &pcie_lstat); to read the Link Status Register, I think
> it should use pcie_capability_read_word(skdev->pdev, PCI_EXP_LNKSTA,
> &pcie_lstat);
> 
> Please let me know if the above changes are correct so I may send a patch.

You just want to print the current link speed, right? Does
skdev->pdev->bus->cur_bus_speed provide what you need?

Re: [PATCH] drivers: block: save return value of pci_find_capability() in u8

[Puranjay Mohan]

On Mon, Dec 7, 2020 at 11:37 PM Keith Busch <kbusch@kernel.org> wrote:
>
> On Mon, Dec 07, 2020 at 11:23:03PM +0530, Puranjay Mohan wrote:
> > On Mon, Dec 7, 2020 at 8:23 PM Christoph Hellwig <hch@infradead.org> wrote:
> > >
> > > Can we take a step back?  I think in general drivers should not bother
> > > with pci_find_capability.  Both mtip32xx and skd want to find out if
> > > the devices are PCIe devices, skd then just prints the link speed for
> > > which we have much better helpers, mtip32xx than messes with DEVCTL
> > > which seems actively dangerous to me.
> >
> > The skd driver uses pci_find_capability(skdev->pdev, PCI_CAP_ID_EXP);
> > to check if the device is PCIe, it can use pci_is_pcie() to do that.
> > After that it uses pci_read_config_word(skdev->pdev, pcie_reg,
> > &pcie_lstat); to read the Link Status Register, I think
> > it should use pcie_capability_read_word(skdev->pdev, PCI_EXP_LNKSTA,
> > &pcie_lstat);
> >
> > Please let me know if the above changes are correct so I may send a patch.
>
> You just want to print the current link speed, right? Does
> skdev->pdev->bus->cur_bus_speed provide what you need?

After discussing with Bjorn, we concluded that we don't need to print
the speed as it is already printed by the PCI core for every device.
PCI core calls __pcie_print_link_status() for every device, and hence
the skd_pci_info() is redundant and can be removed?


-- 
Thanks and Regards

Yours Truly,

Puranjay Mohan