| Message ID | 20230927154339.1600738-11-rrichter@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl/pci: Add support for RCH RAS error handling | expand |
On Wed, 27 Sep 2023 17:43:29 +0200 Robert Richter <rrichter@amd.com> wrote: > CXL error handling depends on AER. > > Introduce config option PCIEAER_CXL in preparation of the AER dport > error handling. Also, introduce the stub function > devm_cxl_setup_parent_dport() to setup dports. > > This is in preparation of follow on patches. > > Note the Kconfg part of the option is added in a later patch to enable > it once coding of the feature is complete. > > Signed-off-by: Robert Richter <rrichter@amd.com> Feels like it should just be combined with a later patch that fills some of this in as on it's own it's just a weird snippet of code :) Still, one comment inline anyway. > --- > drivers/cxl/core/pci.c | 9 +++++++++ > drivers/cxl/cxl.h | 7 +++++++ > drivers/cxl/mem.c | 2 ++ > 3 files changed, 18 insertions(+) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index c7a7887ebdcf..6ba3b7370816 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -718,6 +718,15 @@ static bool cxl_report_and_clear(struct cxl_dev_state *cxlds) > return true; > } > > +#ifdef CONFIG_PCIEAER_CXL > + > +void devm_cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport) > +{ > +} > +EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_parent_dport, CXL); > + > +#endif > + > pci_ers_result_t cxl_error_detected(struct pci_dev *pdev, > pci_channel_state_t state) > { > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index c07064e0c136..cfa2f6bede41 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -704,6 +704,13 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port, > struct device *dport_dev, int port_id, > resource_size_t rcrb); > > +#ifdef CONFIG_PCIEAER_CXL > +void devm_cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport); > +#else > +static inline void devm_cxl_setup_parent_dport(struct device *host, > + struct cxl_dport *dport) { } > +#endif > + > struct cxl_decoder *to_cxl_decoder(struct device *dev); > struct cxl_root_decoder *to_cxl_root_decoder(struct device *dev); > struct cxl_switch_decoder *to_cxl_switch_decoder(struct device *dev); > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index 04107058739b..61ca21c020fa 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -157,6 +157,8 @@ static int cxl_mem_probe(struct device *dev) > else > endpoint_parent = &parent_port->dev; > > + devm_cxl_setup_parent_dport(dev, dport); devm calls can always fail (because if nothing else you have to register some cleanup and that involves an allocation. If you want to ignore that I'd expect a comment here. > + > device_lock(endpoint_parent); > if (!endpoint_parent->driver) { > dev_err(dev, "CXL port topology %s not enabled\n",
Hi Jonathan, I added responses inline below. On 10/2/23 09:46, Jonathan Cameron wrote: > On Wed, 27 Sep 2023 17:43:29 +0200 > Robert Richter <rrichter@amd.com> wrote: > >> CXL error handling depends on AER. >> >> Introduce config option PCIEAER_CXL in preparation of the AER dport >> error handling. Also, introduce the stub function >> devm_cxl_setup_parent_dport() to setup dports. >> >> This is in preparation of follow on patches. >> >> Note the Kconfg part of the option is added in a later patch to enable >> it once coding of the feature is complete. >> >> Signed-off-by: Robert Richter <rrichter@amd.com> > > Feels like it should just be combined with a later patch that fills > some of this in as on it's own it's just a weird snippet of code :) > We will look to merge with the following patch. > Still, one comment inline anyway. > > >> --- >> drivers/cxl/core/pci.c | 9 +++++++++ >> drivers/cxl/cxl.h | 7 +++++++ >> drivers/cxl/mem.c | 2 ++ >> 3 files changed, 18 insertions(+) >> >> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c >> index c7a7887ebdcf..6ba3b7370816 100644 >> --- a/drivers/cxl/core/pci.c >> +++ b/drivers/cxl/core/pci.c >> @@ -718,6 +718,15 @@ static bool cxl_report_and_clear(struct cxl_dev_state *cxlds) >> return true; >> } >> >> +#ifdef CONFIG_PCIEAER_CXL >> + >> +void devm_cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport) >> +{ >> +} >> +EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_parent_dport, CXL); >> + >> +#endif >> + >> pci_ers_result_t cxl_error_detected(struct pci_dev *pdev, >> pci_channel_state_t state) >> { >> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h >> index c07064e0c136..cfa2f6bede41 100644 >> --- a/drivers/cxl/cxl.h >> +++ b/drivers/cxl/cxl.h >> @@ -704,6 +704,13 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port, >> struct device *dport_dev, int port_id, >> resource_size_t rcrb); >> >> +#ifdef CONFIG_PCIEAER_CXL >> +void devm_cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport); >> +#else >> +static inline void devm_cxl_setup_parent_dport(struct device *host, >> + struct cxl_dport *dport) { } >> +#endif >> + >> struct cxl_decoder *to_cxl_decoder(struct device *dev); >> struct cxl_root_decoder *to_cxl_root_decoder(struct device *dev); >> struct cxl_switch_decoder *to_cxl_switch_decoder(struct device *dev); >> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c >> index 04107058739b..61ca21c020fa 100644 >> --- a/drivers/cxl/mem.c >> +++ b/drivers/cxl/mem.c >> @@ -157,6 +157,8 @@ static int cxl_mem_probe(struct device *dev) >> else >> endpoint_parent = &parent_port->dev; >> >> + devm_cxl_setup_parent_dport(dev, dport); > > devm calls can always fail (because if nothing else you have to register > some cleanup and that involves an allocation. If you want to ignore > that I'd expect a comment here. > We will add error handling here. Regards, Terry >> + >> device_lock(endpoint_parent); >> if (!endpoint_parent->driver) { >> dev_err(dev, "CXL port topology %s not enabled\n", >
Hi Jonathan, I added a response below. On 10/9/23 09:44, Terry Bowman wrote: > Hi Jonathan, > > I added responses inline below. > > On 10/2/23 09:46, Jonathan Cameron wrote: >> On Wed, 27 Sep 2023 17:43:29 +0200 >> Robert Richter <rrichter@amd.com> wrote: >> >>> CXL error handling depends on AER. >>> >>> Introduce config option PCIEAER_CXL in preparation of the AER dport >>> error handling. Also, introduce the stub function >>> devm_cxl_setup_parent_dport() to setup dports. >>> >>> This is in preparation of follow on patches. >>> >>> Note the Kconfg part of the option is added in a later patch to enable >>> it once coding of the feature is complete. >>> >>> Signed-off-by: Robert Richter <rrichter@amd.com> >> >> Feels like it should just be combined with a later patch that fills >> some of this in as on it's own it's just a weird snippet of code :) >> > > We will look to merge with the following patch. > >> Still, one comment inline anyway. >> >> >>> --- >>> drivers/cxl/core/pci.c | 9 +++++++++ >>> drivers/cxl/cxl.h | 7 +++++++ >>> drivers/cxl/mem.c | 2 ++ >>> 3 files changed, 18 insertions(+) >>> >>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c >>> index c7a7887ebdcf..6ba3b7370816 100644 >>> --- a/drivers/cxl/core/pci.c >>> +++ b/drivers/cxl/core/pci.c >>> @@ -718,6 +718,15 @@ static bool cxl_report_and_clear(struct cxl_dev_state *cxlds) >>> return true; >>> } >>> >>> +#ifdef CONFIG_PCIEAER_CXL >>> + >>> +void devm_cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport) >>> +{ >>> +} >>> +EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_parent_dport, CXL); >>> + >>> +#endif >>> + >>> pci_ers_result_t cxl_error_detected(struct pci_dev *pdev, >>> pci_channel_state_t state) >>> { >>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h >>> index c07064e0c136..cfa2f6bede41 100644 >>> --- a/drivers/cxl/cxl.h >>> +++ b/drivers/cxl/cxl.h >>> @@ -704,6 +704,13 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port, >>> struct device *dport_dev, int port_id, >>> resource_size_t rcrb); >>> >>> +#ifdef CONFIG_PCIEAER_CXL >>> +void devm_cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport); >>> +#else >>> +static inline void devm_cxl_setup_parent_dport(struct device *host, >>> + struct cxl_dport *dport) { } >>> +#endif >>> + >>> struct cxl_decoder *to_cxl_decoder(struct device *dev); >>> struct cxl_root_decoder *to_cxl_root_decoder(struct device *dev); >>> struct cxl_switch_decoder *to_cxl_switch_decoder(struct device *dev); >>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c >>> index 04107058739b..61ca21c020fa 100644 >>> --- a/drivers/cxl/mem.c >>> +++ b/drivers/cxl/mem.c >>> @@ -157,6 +157,8 @@ static int cxl_mem_probe(struct device *dev) >>> else >>> endpoint_parent = &parent_port->dev; >>> >>> + devm_cxl_setup_parent_dport(dev, dport); >> >> devm calls can always fail (because if nothing else you have to register >> some cleanup and that involves an allocation. If you want to ignore >> that I'd expect a comment here. >> > > We will add error handling here. > > Regards, > Terry > Found devm_cxl_setup_parent_dport() is a NULL function without return value. Regards, Terry
> >>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > >>> index 04107058739b..61ca21c020fa 100644 > >>> --- a/drivers/cxl/mem.c > >>> +++ b/drivers/cxl/mem.c > >>> @@ -157,6 +157,8 @@ static int cxl_mem_probe(struct device *dev) > >>> else > >>> endpoint_parent = &parent_port->dev; > >>> > >>> + devm_cxl_setup_parent_dport(dev, dport); > >> > >> devm calls can always fail (because if nothing else you have to register > >> some cleanup and that involves an allocation. If you want to ignore > >> that I'd expect a comment here. > >> > > > > We will add error handling here. > > > > Regards, > > Terry > > > > Found devm_cxl_setup_parent_dport() is a NULL function without return value. Hmm. Then it's not a devm function. I went looking and can't find it registering any cleanup so just rename it to cxl_setup_parent_dport() > > Regards, > Terry
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index c7a7887ebdcf..6ba3b7370816 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -718,6 +718,15 @@ static bool cxl_report_and_clear(struct cxl_dev_state *cxlds) return true; } +#ifdef CONFIG_PCIEAER_CXL + +void devm_cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport) +{ +} +EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_parent_dport, CXL); + +#endif + pci_ers_result_t cxl_error_detected(struct pci_dev *pdev, pci_channel_state_t state) { diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index c07064e0c136..cfa2f6bede41 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -704,6 +704,13 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port, struct device *dport_dev, int port_id, resource_size_t rcrb); +#ifdef CONFIG_PCIEAER_CXL +void devm_cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport); +#else +static inline void devm_cxl_setup_parent_dport(struct device *host, + struct cxl_dport *dport) { } +#endif + struct cxl_decoder *to_cxl_decoder(struct device *dev); struct cxl_root_decoder *to_cxl_root_decoder(struct device *dev); struct cxl_switch_decoder *to_cxl_switch_decoder(struct device *dev); diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c index 04107058739b..61ca21c020fa 100644 --- a/drivers/cxl/mem.c +++ b/drivers/cxl/mem.c @@ -157,6 +157,8 @@ static int cxl_mem_probe(struct device *dev) else endpoint_parent = &parent_port->dev; + devm_cxl_setup_parent_dport(dev, dport); + device_lock(endpoint_parent); if (!endpoint_parent->driver) { dev_err(dev, "CXL port topology %s not enabled\n",
CXL error handling depends on AER. Introduce config option PCIEAER_CXL in preparation of the AER dport error handling. Also, introduce the stub function devm_cxl_setup_parent_dport() to setup dports. This is in preparation of follow on patches. Note the Kconfg part of the option is added in a later patch to enable it once coding of the feature is complete. Signed-off-by: Robert Richter <rrichter@amd.com> --- drivers/cxl/core/pci.c | 9 +++++++++ drivers/cxl/cxl.h | 7 +++++++ drivers/cxl/mem.c | 2 ++ 3 files changed, 18 insertions(+)