Messages in this thread | | | Date | Thu, 19 Mar 2020 17:52:48 -0500 | From | Bjorn Helgaas <> | Subject | Re: [PATCH v17 06/12] Documentation: PCI: Remove reset_link references |
| |
On Tue, Mar 17, 2020 at 10:06:54AM -0700, Christoph Hellwig wrote: > On Tue, Mar 17, 2020 at 11:03:36AM -0500, Bjorn Helgaas wrote: > > On Tue, Mar 17, 2020 at 10:09 AM Christoph Hellwig <hch@infradead.org> wrote: > > > On Tue, Mar 17, 2020 at 08:05:50AM -0700, Kuppuswamy, Sathyanarayanan wrote: > > > > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > > > > > > > > > This should be folded into the patch removing the method. > > > > This is also folded in the mentioned patch. > > > > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=review/edr&id=7a18dc6506f108db3dc40f5cd779bc15270c4183 > > > > > > I can't find that series anywhere on the list. What did I miss? > > > > We've still been discussing other issues (access to AER registers, > > synchronization between EDR and hotplug, etc) in other parts of this > > thread. The git branch Sathy pointed to above is my local branch. > > I'll send it to the list before putting it into -next, but I wanted to > > make progress on some of these other issues first. > > A few nitpicks: > > PCI/ERR: Update error status after reset_link(): > > - there are two "if (state == pci_channel_io_frozen)" > right after each other now, merging them would make the code a little > easier to read.
Merged, thanks.
> PCI/DPC: Move DPC data into struct pci_dev: > > - dpc_rp_extensions probable should be a "bool : 1"
I actually had not seen "bool : 1" used, but you're right, there are several. There aren't any in drivers/pci, though, so I'm inclined to stay consistent with "unsigned int : 1" unless there's an advantage, and then I'd probably convert all of drivers/pci over.
My rule of thumb has been [1], where Linus suggests "unsigned int percpu:1", but maybe that should be updated.
> PCI/ERR: Remove service dependency in pcie_do_recovery(): > > - as mentioned to Kuppuswamy the reset_cb is never NULL, and thus > a lot of dead code in reset_link can be removed.
Agreed, thanks, I removed that dead code.
> Also reset_link should be merged into pcie_do_recovery. That > would also enable to call the argument reset_link, which might be > a bit more descriptive than reset_cb.
I didn't do this because it sounds like it might be a separate patch. But maybe Sathy can do this in the next round?
> PCI/DPC: Cache DPC capabilities in pci_init_capabilities(): > > - I think the pci_dpc_init could be cleaned up a bit to: > > ... > pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap); > if (!(cap & PCI_EXP_DPC_CAP_RP_EXT)) > return; > pdev->dpc_rp_extensions = true; > pdev->dpc_rp_log_size = (cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8; > ...
Nice, thanks! I made this change, too.
Thanks a lot for reviewing this!
Bjorn
[1] https://lore.kernel.org/linux-arm-kernel/CA+55aFxnePDimkVKVtv3gNmRGcwc8KQ5mHYvUxY8sAQg6yvVYg@mail.gmail.com/
| |