diff mbox series

drivers: block: skd: remove skd_pci_info()

Message ID 20201211164137.8605-1-puranjay12@gmail.com (mailing list archive)
State Superseded
Headers show
Series drivers: block: skd: remove skd_pci_info() | expand

Commit Message

Puranjay Mohan Dec. 11, 2020, 4:41 p.m. UTC
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(-)

Comments

Chaitanya Kulkarni Dec. 11, 2020, 9:50 p.m. UTC | #1
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);
Bjorn Helgaas Dec. 11, 2020, 10:41 p.m. UTC | #2
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);
>
Chaitanya Kulkarni Dec. 12, 2020, 1:59 a.m. UTC | #3
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>
Damien Le Moal Dec. 14, 2020, 6:02 a.m. UTC | #4
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.
diff mbox series

Patch

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);