| Message ID | 20201206194300.14221-1-puranjay12@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | drivers: block: save return value of pci_find_capability() in u8 | expand |
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);
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); > >
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); >> >> >
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>
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>
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.
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.
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
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.
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?
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?
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.
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);
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(-)