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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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);
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 <[email protected]>
> > ---
> > 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);
>
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 <[email protected]>