| Message ID | 20170405085243.18123-1-kishon@ti.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Apr 05, 2017 at 02:22:21PM +0530, Kishon Vijay Abraham I wrote: > Introduce a new EP core layer in order to support endpoint functions in > linux kernel. This comprises the EPC library (Endpoint Controller Library) > and EPF library (Endpoint Function Library). EPC library implements > functions specific to an endpoint controller and EPF library implements > functions specific to an endpoint function. > ... > +/** > + * pci_epf_linkup() - Notify the function driver that EPC device has > + * established a connection with the Root Complex. > + * @epf: the EPF device bound to the EPC device which has established > + * the connection with the host > + * > + * Invoke to notify the function driver that EPC device has established > + * a connection with the Root Complex. > + */ > +void pci_epf_linkup(struct pci_epf *epf) > +{ > + if (!epf->driver) > + dev_WARN(&epf->dev, "epf device not bound to driver\n"); > + > + epf->driver->ops->linkup(epf); I don't understand what's going on here. We warn if epf->driver is NULL, but the next thing we do is dereference it. For NULL pointers that are symptoms of Linux defects, I usually prefer not to check at all so that a dereference generates an oops and we can debug the problem. For NULL pointers caused by user error, we would generally return an error that percolates up to the user. I haven't competely wrapped my head around this endpoint support, but I assume a NULL pointer here would be caused by user error, not necessarily a Linux defect. So why would we dereference a NULL pointer? And what happens when we do? Is this just going to oops an embedded Linux running inside the endpoint? Is that the correct behavior? Bjorn
Hi Bjorn, On Wednesday 05 April 2017 10:22 PM, Bjorn Helgaas wrote: > On Wed, Apr 05, 2017 at 02:22:21PM +0530, Kishon Vijay Abraham I wrote: >> Introduce a new EP core layer in order to support endpoint functions in >> linux kernel. This comprises the EPC library (Endpoint Controller Library) >> and EPF library (Endpoint Function Library). EPC library implements >> functions specific to an endpoint controller and EPF library implements >> functions specific to an endpoint function. >> ... > >> +/** >> + * pci_epf_linkup() - Notify the function driver that EPC device has >> + * established a connection with the Root Complex. >> + * @epf: the EPF device bound to the EPC device which has established >> + * the connection with the host >> + * >> + * Invoke to notify the function driver that EPC device has established >> + * a connection with the Root Complex. >> + */ >> +void pci_epf_linkup(struct pci_epf *epf) >> +{ >> + if (!epf->driver) >> + dev_WARN(&epf->dev, "epf device not bound to driver\n"); >> + >> + epf->driver->ops->linkup(epf); > > I don't understand what's going on here. We warn if epf->driver is > NULL, but the next thing we do is dereference it. > > For NULL pointers that are symptoms of Linux defects, I usually prefer > not to check at all so that a dereference generates an oops and we can > debug the problem. For NULL pointers caused by user error, we would > generally return an error that percolates up to the user. > > I haven't competely wrapped my head around this endpoint support, but > I assume a NULL pointer here would be caused by user error, not > necessarily a Linux defect. So why would we dereference a NULL > pointer? And what happens when we do? Is this just going to oops an > embedded Linux running inside the endpoint? Is that the correct > behavior? With the new configfs directory structure, this should be a kernel error. However the EPF layer should be independent of how it's API's are used i.e someone can create a new sysfs/configfs structure and the value of epf->driver might be dependent on user actions. I think I'd prefer not to dereference NULL pointers since we anyways have a dev_WARN for debug. I'll resend this patch with return if epf->driver is NULL. Thanks Kishon
On Wed, Apr 05, 2017 at 02:22:20PM +0530, Kishon Vijay Abraham I wrote: > Hi Bjorn, > > Please find the pull request for PCI endpoint support below. I've > also included all the history here. Thanks, I applied these (with v7 of the first patch) to pci/host-designware for v4.12. > Changes from v5: > *) remove #syscon-cells property added in v5 and used > of_parse_phandle_with_fixed_args > *) fix compilation error in make.cross ARCH=blackfin that was because > pci_endpoint_test.c driver depends on COMPILE_TEST. > > Changes from v4: > *) add #syscon-cells property and used of_parse_phandle_with_args > to perform a configuration in syscon module (as suggested by > Rob Herring) > *) Remove unnecessary white space. > > Changes from v3: > *) fixed a typo and adapted to https://lkml.org/lkml/2017/3/13/562. > > Changes from v2: > *) changed the configfs structure as suggested by Christoph Hellwig. With > this change the framework creates configfs entry for EP function driver > and EP controller. Previously these entries have to be created by the > the user. (Haven't changed the epc core or epf core except for invoking > configfs APIs to create entries for EP function driver and EP controller. > That's mostly because the EP function device can still be created by > directly invoking the epf core API without using configfs). > *) Now the user has to use configfs entry 'start' to start the link. > This was previously done by the function driver. However in the case of > multi function EP, the function driver shouldn't start the link. > > Changes from v1: > *) The preparation patches for adding EP support is removed and is sent > separately > *) Added device ID for DRA74x/DRA72x and used it instead of > using "PCI_ANY_ID" > *) Added userguide for PCI endpoint test function > > Major Improvements from RFC: > *) support multi-function devices (hw supported not virtual) > *) Access host side buffers > *) Raise MSI interrupts > *) Add user space program to use the host side PCI driver > *) Adapt all other users of designware to use the new design (only > compile tested. Since I have only dra7xx boards, the new design > has only been tested in dra7xx. I'd require the help of others > to test the platforms they have access to). > > This is based on > git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git > pci/host-designware > > Thanks > Kishon > > The following changes since commit 7ea64dcf602c21b3e5062ca90111ca4459fab403: > > __end__ (2017-04-04 15:29:37 -0500) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/kishon/pci-endpoint.git tags/pci-endpoint-for-4.12 > > for you to fetch changes up to a5c85ba45c9682456077d7277196e91f8ea5fd5c: > > ARM: DRA7: clockdomain: Change the CLKTRCTRL of CM_PCIE_CLKSTCTRL to SW_WKUP (2017-04-05 14:05:28 +0530) > > ---------------------------------------------------------------- > pci: endpoint: for 4.12 > > *) Add PCI endpoint core layer > *) Modify designware and dra7xx driver to be configured in EP mode > *) Add a PCI endpoint *test* function driver and corresponding host > driver > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > > ---------------------------------------------------------------- > Kishon Vijay Abraham I (23): > PCI: endpoint: Add EP core layer to enable EP controller and EP functions > Documentation: PCI: Guide to use PCI Endpoint Core Layer > PCI: endpoint: Introduce configfs entry for configuring EP functions > Documentation: PCI: Guide to use PCI endpoint configfs > PCI: endpoint: Create configfs entry for EPC device and EPF driver > Documentation: PCI: Add specification for the *PCI test* function device > PCI: endpoint: functions: Add an EP function to test PCI > Documentation: PCI: Add binding documentation for pci-test endpoint function > PCI: dwc: designware: Add EP mode support > dt-bindings: PCI: Add DT bindings for PCI designware EP mode > PCI: dwc: dra7xx: Facilitate wrapper and MSI interrupts to be enabled independently > PCI: dwc: dra7xx: Add EP mode support > dt-bindings: PCI: dra7xx: Add DT bindings for PCI dra7xx EP mode > PCI: dwc: dra7xx: Workaround for errata id i870 > dt-bindings: PCI: dra7xx: Add DT bindings to enable unaligned access > PCI: Add device IDs for DRA74x and DRA72x > misc: Add host side PCI driver for PCI test function device > Documentation: misc-devices: Add Documentation for pci-endpoint-test driver > tools: PCI: Add a userspace tool to test PCI endpoint > tools: PCI: Add sample test script to invoke pcitest > Documentation: PCI: Add userguide for PCI endpoint test function > MAINTAINERS: Add PCI Endpoint maintainer > ARM: DRA7: clockdomain: Change the CLKTRCTRL of CM_PCIE_CLKSTCTRL to SW_WKUP > > Documentation/PCI/00-INDEX | 10 +++ > Documentation/PCI/endpoint/function/binding/pci-test.txt | 17 +++++ > Documentation/PCI/endpoint/pci-endpoint-cfs.txt | 105 ++++++++++++++++++++++++++ > Documentation/PCI/endpoint/pci-endpoint.txt | 215 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > Documentation/PCI/endpoint/pci-test-function.txt | 66 ++++++++++++++++ > Documentation/PCI/endpoint/pci-test-howto.txt | 179 +++++++++++++++++++++++++++++++++++++++++++ > Documentation/devicetree/bindings/pci/designware-pcie.txt | 26 +++++-- > Documentation/devicetree/bindings/pci/ti-pci.txt | 42 +++++++++-- > Documentation/misc-devices/pci-endpoint-test.txt | 35 +++++++++ > MAINTAINERS | 9 +++ > arch/arm/mach-omap2/clockdomains7xx_data.c | 2 +- > drivers/Makefile | 2 + > drivers/misc/Kconfig | 7 ++ > drivers/misc/Makefile | 1 + > drivers/misc/pci_endpoint_test.c | 534 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/pci/Kconfig | 1 + > drivers/pci/dwc/Kconfig | 36 ++++++++- > drivers/pci/dwc/Makefile | 5 +- > drivers/pci/dwc/pci-dra7xx.c | 270 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----- > drivers/pci/dwc/pcie-designware-ep.c | 342 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/pci/dwc/pcie-designware.c | 125 ++++++++++++++++++++++++++++++ > drivers/pci/dwc/pcie-designware.h | 112 +++++++++++++++++++++++++++ > drivers/pci/endpoint/Kconfig | 31 ++++++++ > drivers/pci/endpoint/Makefile | 7 ++ > drivers/pci/endpoint/functions/Kconfig | 12 +++ > drivers/pci/endpoint/functions/Makefile | 5 ++ > drivers/pci/endpoint/functions/pci-epf-test.c | 510 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/pci/endpoint/pci-ep-cfs.c | 509 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/pci/endpoint/pci-epc-core.c | 579 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/pci/endpoint/pci-epc-mem.c | 143 +++++++++++++++++++++++++++++++++++ > drivers/pci/endpoint/pci-epf-core.c | 351 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/mod_devicetable.h | 10 +++ > include/linux/pci-ep-cfs.h | 41 ++++++++++ > include/linux/pci-epc.h | 144 +++++++++++++++++++++++++++++++++++ > include/linux/pci-epf.h | 162 +++++++++++++++++++++++++++++++++++++++ > include/linux/pci_ids.h | 2 + > include/uapi/linux/Kbuild | 1 + > include/uapi/linux/pcitest.h | 19 +++++ > tools/pci/pcitest.c | 186 +++++++++++++++++++++++++++++++++++++++++++++ > tools/pci/pcitest.sh | 56 ++++++++++++++ > 40 files changed, 4869 insertions(+), 40 deletions(-) > create mode 100644 Documentation/PCI/endpoint/function/binding/pci-test.txt > create mode 100644 Documentation/PCI/endpoint/pci-endpoint-cfs.txt > create mode 100644 Documentation/PCI/endpoint/pci-endpoint.txt > create mode 100644 Documentation/PCI/endpoint/pci-test-function.txt > create mode 100644 Documentation/PCI/endpoint/pci-test-howto.txt > create mode 100644 Documentation/misc-devices/pci-endpoint-test.txt > create mode 100644 drivers/misc/pci_endpoint_test.c > create mode 100644 drivers/pci/dwc/pcie-designware-ep.c > create mode 100644 drivers/pci/endpoint/Kconfig > create mode 100644 drivers/pci/endpoint/Makefile > create mode 100644 drivers/pci/endpoint/functions/Kconfig > create mode 100644 drivers/pci/endpoint/functions/Makefile > create mode 100644 drivers/pci/endpoint/functions/pci-epf-test.c > create mode 100644 drivers/pci/endpoint/pci-ep-cfs.c > create mode 100644 drivers/pci/endpoint/pci-epc-core.c > create mode 100644 drivers/pci/endpoint/pci-epc-mem.c > create mode 100644 drivers/pci/endpoint/pci-epf-core.c > create mode 100644 include/linux/pci-ep-cfs.h > create mode 100644 include/linux/pci-epc.h > create mode 100644 include/linux/pci-epf.h > create mode 100644 include/uapi/linux/pcitest.h > create mode 100644 tools/pci/pcitest.c > create mode 100644 tools/pci/pcitest.sh > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Bjorn, On Wednesday 12 April 2017 01:04 AM, Bjorn Helgaas wrote: > On Mon, Apr 10, 2017 at 10:43:28AM -0500, Bjorn Helgaas wrote: >> On Wed, Apr 05, 2017 at 02:22:20PM +0530, Kishon Vijay Abraham I wrote: >>> Hi Bjorn, >>> >>> Please find the pull request for PCI endpoint support below. I've >>> also included all the history here. >> >> Thanks, I applied these (with v7 of the first patch) to pci/host-designware >> for v4.12. > > Ok, sorry, I screwed this up. I think my branch actually had v5, not > v6. But I *think* I fixed it. Here's the diff from my branch to your > git tree. Apparently you haven't pushed the v7 patch there, so I > *think* the diff below is the diff between v6 and v7 of that first > patch. I just checked your pci/host-designware branch and it looks correct. Thanks for sorting this out. Cheers Kishon
Hi Bjorn, Please find the pull request for PCI endpoint support below. I've also included all the history here. Changes from v5: *) remove #syscon-cells property added in v5 and used of_parse_phandle_with_fixed_args *) fix compilation error in make.cross ARCH=blackfin that was because pci_endpoint_test.c driver depends on COMPILE_TEST. Changes from v4: *) add #syscon-cells property and used of_parse_phandle_with_args to perform a configuration in syscon module (as suggested by Rob Herring) *) Remove unnecessary white space. Changes from v3: *) fixed a typo and adapted to https://lkml.org/lkml/2017/3/13/562. Changes from v2: *) changed the configfs structure as suggested by Christoph Hellwig. With this change the framework creates configfs entry for EP function driver and EP controller. Previously these entries have to be created by the the user. (Haven't changed the epc core or epf core except for invoking configfs APIs to create entries for EP function driver and EP controller. That's mostly because the EP function device can still be created by directly invoking the epf core API without using configfs). *) Now the user has to use configfs entry 'start' to start the link. This was previously done by the function driver. However in the case of multi function EP, the function driver shouldn't start the link. Changes from v1: *) The preparation patches for adding EP support is removed and is sent separately *) Added device ID for DRA74x/DRA72x and used it instead of using "PCI_ANY_ID" *) Added userguide for PCI endpoint test function Major Improvements from RFC: *) support multi-function devices (hw supported not virtual) *) Access host side buffers *) Raise MSI interrupts *) Add user space program to use the host side PCI driver *) Adapt all other users of designware to use the new design (only compile tested. Since I have only dra7xx boards, the new design has only been tested in dra7xx. I'd require the help of others to test the platforms they have access to). This is based on git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git pci/host-designware Thanks Kishon The following changes since commit 7ea64dcf602c21b3e5062ca90111ca4459fab403: __end__ (2017-04-04 15:29:37 -0500) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/kishon/pci-endpoint.git tags/pci-endpoint-for-4.12 for you to fetch changes up to a5c85ba45c9682456077d7277196e91f8ea5fd5c: ARM: DRA7: clockdomain: Change the CLKTRCTRL of CM_PCIE_CLKSTCTRL to SW_WKUP (2017-04-05 14:05:28 +0530) ---------------------------------------------------------------- pci: endpoint: for 4.12 *) Add PCI endpoint core layer *) Modify designware and dra7xx driver to be configured in EP mode *) Add a PCI endpoint *test* function driver and corresponding host driver Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> ---------------------------------------------------------------- Kishon Vijay Abraham I (23): PCI: endpoint: Add EP core layer to enable EP controller and EP functions Documentation: PCI: Guide to use PCI Endpoint Core Layer PCI: endpoint: Introduce configfs entry for configuring EP functions Documentation: PCI: Guide to use PCI endpoint configfs PCI: endpoint: Create configfs entry for EPC device and EPF driver Documentation: PCI: Add specification for the *PCI test* function device PCI: endpoint: functions: Add an EP function to test PCI Documentation: PCI: Add binding documentation for pci-test endpoint function PCI: dwc: designware: Add EP mode support dt-bindings: PCI: Add DT bindings for PCI designware EP mode PCI: dwc: dra7xx: Facilitate wrapper and MSI interrupts to be enabled independently PCI: dwc: dra7xx: Add EP mode support dt-bindings: PCI: dra7xx: Add DT bindings for PCI dra7xx EP mode PCI: dwc: dra7xx: Workaround for errata id i870 dt-bindings: PCI: dra7xx: Add DT bindings to enable unaligned access PCI: Add device IDs for DRA74x and DRA72x misc: Add host side PCI driver for PCI test function device Documentation: misc-devices: Add Documentation for pci-endpoint-test driver tools: PCI: Add a userspace tool to test PCI endpoint tools: PCI: Add sample test script to invoke pcitest Documentation: PCI: Add userguide for PCI endpoint test function MAINTAINERS: Add PCI Endpoint maintainer ARM: DRA7: clockdomain: Change the CLKTRCTRL of CM_PCIE_CLKSTCTRL to SW_WKUP Documentation/PCI/00-INDEX | 10 +++ Documentation/PCI/endpoint/function/binding/pci-test.txt | 17 +++++ Documentation/PCI/endpoint/pci-endpoint-cfs.txt | 105 ++++++++++++++++++++++++++ Documentation/PCI/endpoint/pci-endpoint.txt | 215 ++++++++++++++++++++++++++++++++++++++++++++++++++++ Documentation/PCI/endpoint/pci-test-function.txt | 66 ++++++++++++++++ Documentation/PCI/endpoint/pci-test-howto.txt | 179 +++++++++++++++++++++++++++++++++++++++++++ Documentation/devicetree/bindings/pci/designware-pcie.txt | 26 +++++-- Documentation/devicetree/bindings/pci/ti-pci.txt | 42 +++++++++-- Documentation/misc-devices/pci-endpoint-test.txt | 35 +++++++++ MAINTAINERS | 9 +++ arch/arm/mach-omap2/clockdomains7xx_data.c | 2 +- drivers/Makefile | 2 + drivers/misc/Kconfig | 7 ++ drivers/misc/Makefile | 1 + drivers/misc/pci_endpoint_test.c | 534 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/pci/Kconfig | 1 + drivers/pci/dwc/Kconfig | 36 ++++++++- drivers/pci/dwc/Makefile | 5 +- drivers/pci/dwc/pci-dra7xx.c | 270 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----- drivers/pci/dwc/pcie-designware-ep.c | 342 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/pci/dwc/pcie-designware.c | 125 ++++++++++++++++++++++++++++++ drivers/pci/dwc/pcie-designware.h | 112 +++++++++++++++++++++++++++ drivers/pci/endpoint/Kconfig | 31 ++++++++ drivers/pci/endpoint/Makefile | 7 ++ drivers/pci/endpoint/functions/Kconfig | 12 +++ drivers/pci/endpoint/functions/Makefile | 5 ++ drivers/pci/endpoint/functions/pci-epf-test.c | 510 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/pci/endpoint/pci-ep-cfs.c | 509 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/pci/endpoint/pci-epc-core.c | 579 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/pci/endpoint/pci-epc-mem.c | 143 +++++++++++++++++++++++++++++++++++ drivers/pci/endpoint/pci-epf-core.c | 351 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/mod_devicetable.h | 10 +++ include/linux/pci-ep-cfs.h | 41 ++++++++++ include/linux/pci-epc.h | 144 +++++++++++++++++++++++++++++++++++ include/linux/pci-epf.h | 162 +++++++++++++++++++++++++++++++++++++++ include/linux/pci_ids.h | 2 + include/uapi/linux/Kbuild | 1 + include/uapi/linux/pcitest.h | 19 +++++ tools/pci/pcitest.c | 186 +++++++++++++++++++++++++++++++++++++++++++++ tools/pci/pcitest.sh | 56 ++++++++++++++ 40 files changed, 4869 insertions(+), 40 deletions(-) create mode 100644 Documentation/PCI/endpoint/function/binding/pci-test.txt create mode 100644 Documentation/PCI/endpoint/pci-endpoint-cfs.txt create mode 100644 Documentation/PCI/endpoint/pci-endpoint.txt create mode 100644 Documentation/PCI/endpoint/pci-test-function.txt create mode 100644 Documentation/PCI/endpoint/pci-test-howto.txt create mode 100644 Documentation/misc-devices/pci-endpoint-test.txt create mode 100644 drivers/misc/pci_endpoint_test.c create mode 100644 drivers/pci/dwc/pcie-designware-ep.c create mode 100644 drivers/pci/endpoint/Kconfig create mode 100644 drivers/pci/endpoint/Makefile create mode 100644 drivers/pci/endpoint/functions/Kconfig create mode 100644 drivers/pci/endpoint/functions/Makefile create mode 100644 drivers/pci/endpoint/functions/pci-epf-test.c create mode 100644 drivers/pci/endpoint/pci-ep-cfs.c create mode 100644 drivers/pci/endpoint/pci-epc-core.c create mode 100644 drivers/pci/endpoint/pci-epc-mem.c create mode 100644 drivers/pci/endpoint/pci-epf-core.c create mode 100644 include/linux/pci-ep-cfs.h create mode 100644 include/linux/pci-epc.h create mode 100644 include/linux/pci-epf.h create mode 100644 include/uapi/linux/pcitest.h create mode 100644 tools/pci/pcitest.c create mode 100644 tools/pci/pcitest.sh