* [PATCH] drivers: block: skd: remove skd_pci_info()
@ 2020-12-11 16:41 Puranjay Mohan
2020-12-11 21:50 ` Chaitanya Kulkarni
2020-12-14 6:02 ` Damien Le Moal
0 siblings, 2 replies; 5+ messages in thread
From: Puranjay Mohan @ 2020-12-11 16:41 UTC (permalink / raw)
To: bjorn, linux-pci, Damien.LeMoal, axboe, linux-block, linux-kernel
Cc: Puranjay Mohan
PCI core calls __pcie_print_link_status() for every device, it prints
both the link width and the link speed. skd_pci_info() does the same
thing again, hence it can be removed.
Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
---
drivers/block/skd_main.c | 31 -------------------------------
1 file changed, 31 deletions(-)
diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
index a962b4551bed..da7aac5335d9 100644
--- a/drivers/block/skd_main.c
+++ b/drivers/block/skd_main.c
@@ -3134,40 +3134,11 @@ static const struct pci_device_id skd_pci_tbl[] = {
MODULE_DEVICE_TABLE(pci, skd_pci_tbl);
-static char *skd_pci_info(struct skd_device *skdev, char *str)
-{
- int pcie_reg;
-
- strcpy(str, "PCIe (");
- pcie_reg = pci_find_capability(skdev->pdev, PCI_CAP_ID_EXP);
-
- if (pcie_reg) {
-
- char lwstr[6];
- uint16_t pcie_lstat, lspeed, lwidth;
-
- pcie_reg += 0x12;
- pci_read_config_word(skdev->pdev, pcie_reg, &pcie_lstat);
- lspeed = pcie_lstat & (0xF);
- lwidth = (pcie_lstat & 0x3F0) >> 4;
-
- if (lspeed == 1)
- strcat(str, "2.5GT/s ");
- else if (lspeed == 2)
- strcat(str, "5.0GT/s ");
- else
- strcat(str, "<unknown> ");
- snprintf(lwstr, sizeof(lwstr), "%dX)", lwidth);
- strcat(str, lwstr);
- }
- return str;
-}
static int skd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
{
int i;
int rc = 0;
- char pci_str[32];
struct skd_device *skdev;
dev_dbg(&pdev->dev, "vendor=%04X device=%04x\n", pdev->vendor,
@@ -3201,8 +3172,6 @@ static int skd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
goto err_out_regions;
}
- skd_pci_info(skdev, pci_str);
- dev_info(&pdev->dev, "%s 64bit\n", pci_str);
pci_set_master(pdev);
rc = pci_enable_pcie_error_reporting(pdev);
--
2.27.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] drivers: block: skd: remove skd_pci_info()
2020-12-11 16:41 [PATCH] drivers: block: skd: remove skd_pci_info() Puranjay Mohan
@ 2020-12-11 21:50 ` Chaitanya Kulkarni
2020-12-11 22:41 ` Bjorn Helgaas
2020-12-14 6:02 ` Damien Le Moal
1 sibling, 1 reply; 5+ messages in thread
From: Chaitanya Kulkarni @ 2020-12-11 21:50 UTC (permalink / raw)
To: Puranjay Mohan, bjorn@helgaas.com, linux-pci@vger.kernel.org,
Damien Le Moal, axboe@kernel.dk, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org
On 12/11/20 08:45, Puranjay Mohan wrote:
> PCI core calls __pcie_print_link_status() for every device, it prints
> both the link width and the link speed. skd_pci_info() does the same
> thing again, hence it can be removed.
>
> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> ---
> drivers/block/skd_main.c | 31 -------------------------------
> 1 file changed, 31 deletions(-)
>
> diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
> index a962b4551bed..da7aac5335d9 100644
> --- a/drivers/block/skd_main.c
> +++ b/drivers/block/skd_main.c
> @@ -3134,40 +3134,11 @@ static const struct pci_device_id skd_pci_tbl[] = {
>
> MODULE_DEVICE_TABLE(pci, skd_pci_tbl);
>
> -static char *skd_pci_info(struct skd_device *skdev, char *str)
> -{
> - int pcie_reg;
> -
> - strcpy(str, "PCIe (");
> - pcie_reg = pci_find_capability(skdev->pdev, PCI_CAP_ID_EXP);
> -
> - if (pcie_reg) {
> -
> - char lwstr[6];
> - uint16_t pcie_lstat, lspeed, lwidth;
> -
> - pcie_reg += 0x12;
> - pci_read_config_word(skdev->pdev, pcie_reg, &pcie_lstat);
> - lspeed = pcie_lstat & (0xF);
> - lwidth = (pcie_lstat & 0x3F0) >> 4;
> -
> - if (lspeed == 1)
> - strcat(str, "2.5GT/s ");
> - else if (lspeed == 2)
> - strcat(str, "5.0GT/s ");
> - else
> - strcat(str, "<unknown> ");
The skd driver prints unknown if the speed is not "2.5GT/s" or "5.0GT/s".
__pcie_print_link_status() prints "unknown" only if speed
value >= ARRAY_SIZE(speed_strings).
If a buggy skd card returns value that is not != ("2.5GT/s" or "5.0GT/s")
&& value < ARRAY_SIZE(speed_strings) then it will not print the unknown but
the value from speed string array.
Which breaks the current behavior. Please correct me if I'm wrong.
> - snprintf(lwstr, sizeof(lwstr), "%dX)", lwidth);
> - strcat(str, lwstr);
> - }
> - return str;
> -}
>
> static int skd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> int i;
> int rc = 0;
> - char pci_str[32];
> struct skd_device *skdev;
>
> dev_dbg(&pdev->dev, "vendor=%04X device=%04x\n", pdev->vendor,
> @@ -3201,8 +3172,6 @@ static int skd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> goto err_out_regions;
> }
>
> - skd_pci_info(skdev, pci_str);
> - dev_info(&pdev->dev, "%s 64bit\n", pci_str);
>
> pci_set_master(pdev);
> rc = pci_enable_pcie_error_reporting(pdev);
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drivers: block: skd: remove skd_pci_info()
2020-12-11 21:50 ` Chaitanya Kulkarni
@ 2020-12-11 22:41 ` Bjorn Helgaas
2020-12-12 1:59 ` Chaitanya Kulkarni
0 siblings, 1 reply; 5+ messages in thread
From: Bjorn Helgaas @ 2020-12-11 22:41 UTC (permalink / raw)
To: Chaitanya Kulkarni
Cc: Puranjay Mohan, bjorn@helgaas.com, linux-pci@vger.kernel.org,
Damien Le Moal, axboe@kernel.dk, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org
On Fri, Dec 11, 2020 at 09:50:52PM +0000, Chaitanya Kulkarni wrote:
> On 12/11/20 08:45, Puranjay Mohan wrote:
> > PCI core calls __pcie_print_link_status() for every device, it prints
> > both the link width and the link speed. skd_pci_info() does the same
> > thing again, hence it can be removed.
> >
> > Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> > ---
> > drivers/block/skd_main.c | 31 -------------------------------
> > 1 file changed, 31 deletions(-)
> >
> > diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
> > index a962b4551bed..da7aac5335d9 100644
> > --- a/drivers/block/skd_main.c
> > +++ b/drivers/block/skd_main.c
> > @@ -3134,40 +3134,11 @@ static const struct pci_device_id skd_pci_tbl[] = {
> >
> > MODULE_DEVICE_TABLE(pci, skd_pci_tbl);
> >
> > -static char *skd_pci_info(struct skd_device *skdev, char *str)
> > -{
> > - int pcie_reg;
> > -
> > - strcpy(str, "PCIe (");
> > - pcie_reg = pci_find_capability(skdev->pdev, PCI_CAP_ID_EXP);
> > -
> > - if (pcie_reg) {
> > -
> > - char lwstr[6];
> > - uint16_t pcie_lstat, lspeed, lwidth;
> > -
> > - pcie_reg += 0x12;
> > - pci_read_config_word(skdev->pdev, pcie_reg, &pcie_lstat);
> > - lspeed = pcie_lstat & (0xF);
> > - lwidth = (pcie_lstat & 0x3F0) >> 4;
> > -
> > - if (lspeed == 1)
> > - strcat(str, "2.5GT/s ");
> > - else if (lspeed == 2)
> > - strcat(str, "5.0GT/s ");
> > - else
> > - strcat(str, "<unknown> ");
> The skd driver prints unknown if the speed is not "2.5GT/s" or "5.0GT/s".
> __pcie_print_link_status() prints "unknown" only if speed
> value >= ARRAY_SIZE(speed_strings).
>
> If a buggy skd card returns value that is not != ("2.5GT/s" or "5.0GT/s")
> && value < ARRAY_SIZE(speed_strings) then it will not print the unknown but
> the value from speed string array.
>
> Which breaks the current behavior. Please correct me if I'm wrong.
I think you're right, but I don't think it actually *breaks* anything.
For skd devices that work correctly, there should be no problem, and
if there ever were an skd device that operated at a speed greater than
5GT/s, the PCI core would print that speed correctly instead of having
the driver print "<unknown>".
I don't think it's a good idea to have a driver artificially constrain
the speed a device operates at. And the existing code doesn't
actually constrain anything; it only prints "<unknown>" if it doesn't
recognize it. The probe still succeeds. I don't see much value in
that "<unknown>".
Or am I missing an actual problem this patch causes?
> > - snprintf(lwstr, sizeof(lwstr), "%dX)", lwidth);
> > - strcat(str, lwstr);
> > - }
> > - return str;
> > -}
> >
> > static int skd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > {
> > int i;
> > int rc = 0;
> > - char pci_str[32];
> > struct skd_device *skdev;
> >
> > dev_dbg(&pdev->dev, "vendor=%04X device=%04x\n", pdev->vendor,
> > @@ -3201,8 +3172,6 @@ static int skd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > goto err_out_regions;
> > }
> >
> > - skd_pci_info(skdev, pci_str);
> > - dev_info(&pdev->dev, "%s 64bit\n", pci_str);
> >
> > pci_set_master(pdev);
> > rc = pci_enable_pcie_error_reporting(pdev);
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drivers: block: skd: remove skd_pci_info()
2020-12-11 22:41 ` Bjorn Helgaas
@ 2020-12-12 1:59 ` Chaitanya Kulkarni
0 siblings, 0 replies; 5+ messages in thread
From: Chaitanya Kulkarni @ 2020-12-12 1:59 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Puranjay Mohan, bjorn@helgaas.com, linux-pci@vger.kernel.org,
Damien Le Moal, axboe@kernel.dk, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org
On 12/11/20 14:41, Bjorn Helgaas wrote:
>> The skd driver prints unknown if the speed is not "2.5GT/s" or "5.0GT/s".
>> __pcie_print_link_status() prints "unknown" only if speed
>> value >= ARRAY_SIZE(speed_strings).
>>
>> If a buggy skd card returns value that is not != ("2.5GT/s" or "5.0GT/s")
>> && value < ARRAY_SIZE(speed_strings) then it will not print the unknown but
>> the value from speed string array.
>>
>> Which breaks the current behavior. Please correct me if I'm wrong.
> I think you're right, but I don't think it actually *breaks* anything.
>
> For skd devices that work correctly, there should be no problem, and
> if there ever were an skd device that operated at a speed greater than
> 5GT/s, the PCI core would print that speed correctly instead of having
> the driver print "<unknown>".
That is the scenario I'm not aware why it prints unknown to start with
and I couldn't find any documentation also, that is why the concern.
> I don't think it's a good idea to have a driver artificially constrain
> the speed a device operates at. And the existing code doesn't
> actually constrain anything; it only prints "<unknown>" if it doesn't
> recognize it. The probe still succeeds. I don't see much value in
> that "<unknown>".
>
> Or am I missing an actual problem this patch causes?
You are not, I'm just not sure without any documentation why does
it print "unknown" and I attributed that to probable firmware issue
(since we all knowhow creative firmware can get ;)).
That makes it the problem with original code more so than with this patch.
In that case I was proposing just keep the original behavior.
But maybe we should apply patch and if any user(s) comes up with the problem
then we can deal with it.
Whoever is going to apply they can add :-
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drivers: block: skd: remove skd_pci_info()
2020-12-11 16:41 [PATCH] drivers: block: skd: remove skd_pci_info() Puranjay Mohan
2020-12-11 21:50 ` Chaitanya Kulkarni
@ 2020-12-14 6:02 ` Damien Le Moal
1 sibling, 0 replies; 5+ messages in thread
From: Damien Le Moal @ 2020-12-14 6:02 UTC (permalink / raw)
To: puranjay12@gmail.com, linux-pci@vger.kernel.org, axboe@kernel.dk,
bjorn@helgaas.com, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org
On Fri, 2020-12-11 at 22:11 +0530, Puranjay Mohan wrote:
> PCI core calls __pcie_print_link_status() for every device, it prints
> both the link width and the link speed. skd_pci_info() does the same
> thing again, hence it can be removed.
Hmmm... On my box, I see this for the skd card:
[ 8.509243] pci 0000:d8:00.0: [1b39:0001] type 00 class 0x018000
[ 8.515933] pci 0000:d8:00.0: reg 0x10: [mem 0xfbe00000-0xfbe0ffff]
[ 8.521924] pci 0000:d8:00.0: reg 0x14: [mem 0xfbe10000-0xfbe10fff]
[ 8.527957] pci 0000:d8:00.0: reg 0x30: [mem 0xfbd00000-0xfbdfffff
pref]
[ 8.534999] pci 0000:d8:00.0: supports D1 D2
No link speed. Checking the code, I think you need to actually call
pcie_print_link_status() (which calls __pcie_print_link_status() with
verbose = true) from the driver to see anything. Otherwise, the PCIe
core will not print anything if the driver is just probing and getting
resources for the card.
>
> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> ---
> drivers/block/skd_main.c | 31 -------------------------------
> 1 file changed, 31 deletions(-)
>
> diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
> index a962b4551bed..da7aac5335d9 100644
> --- a/drivers/block/skd_main.c
> +++ b/drivers/block/skd_main.c
[...]
> static int skd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> int i;
> int rc = 0;
> - char pci_str[32];
> struct skd_device *skdev;
>
> dev_dbg(&pdev->dev, "vendor=%04X device=%04x\n", pdev->vendor,
> @@ -3201,8 +3172,6 @@ static int skd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> goto err_out_regions;
> }
>
> - skd_pci_info(skdev, pci_str);
> - dev_info(&pdev->dev, "%s 64bit\n", pci_str);
Replace these 2 lines with:
pcie_print_link_status(pdev);
And the link speed information will be printed.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-14 6:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-11 16:41 [PATCH] drivers: block: skd: remove skd_pci_info() Puranjay Mohan
2020-12-11 21:50 ` Chaitanya Kulkarni
2020-12-11 22:41 ` Bjorn Helgaas
2020-12-12 1:59 ` Chaitanya Kulkarni
2020-12-14 6:02 ` Damien Le Moal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).