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