| Message ID | 20241203162112.5088-1-alucerop@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv4] cxl: avoid driver data for obtaining cxl_dev_state reference | expand |
On Tue, 3 Dec 2024 16:21:12 +0000 <alucerop@amd.com> wrote: > From: Alejandro Lucero <alucerop@amd.com> > > CXL Type3 pci driver uses struct device driver_data for keeping > cxl_dev_state reference. Type1/2 drivers are not only about CXL so this > field should not be used when code requires cxl_dev_state to work with > and such a code used for Type2 support. > > Change cxl_dvsec_rr_decode for passing cxl_dev_state as a parameter. > > Seize the change for removing the unused cxl_port param. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> I wondered if the other places this assumption is made about drvdata would cause you trouble, but they seem ok for now at least as error handling code you probably won't use. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
On 12/3/24 17:14, Jonathan Cameron wrote: > On Tue, 3 Dec 2024 16:21:12 +0000 > <alucerop@amd.com> wrote: > >> From: Alejandro Lucero <alucerop@amd.com> >> >> CXL Type3 pci driver uses struct device driver_data for keeping >> cxl_dev_state reference. Type1/2 drivers are not only about CXL so this >> field should not be used when code requires cxl_dev_state to work with >> and such a code used for Type2 support. >> >> Change cxl_dvsec_rr_decode for passing cxl_dev_state as a parameter. >> >> Seize the change for removing the unused cxl_port param. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> > I wondered if the other places this assumption is made about drvdata > would cause you trouble, but they seem ok for now at least as > error handling code you probably won't use. I think once the Type2 support is hopefully merged soon, it will be in the heads of all of us for avoiding this kind of uses. For the sake of that support, this patch is enough, but I bet we will find similar issues soon. > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Thanks!
On Tue, Dec 03, 2024 at 04:21:12PM +0000, alucerop@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.com> > > CXL Type3 pci driver uses struct device driver_data for keeping > cxl_dev_state reference. Type1/2 drivers are not only about CXL so this > field should not be used when code requires cxl_dev_state to work with > and such a code used for Type2 support. > > Change cxl_dvsec_rr_decode for passing cxl_dev_state as a parameter. > > Seize the change for removing the unused cxl_port param. > Dave, Jonathan previously offered Reviewed-by tag so this looks good to apply. I've added another review tag and also massaged the changelog for your consideration. cxl/pci: Add Type 1/2 support to cxl_dvsec_rr_decode() In cxl_dvsec_rr_decode() the pci driver expects to retrieve a cxlds, struct cxl_dev_state, from the driver_data field of struct device. While that works for Type 3, drivers for Type 1/2 devices may not put a cxlds in the driver_data field. In preparation for supporting Type 1/2 devices, replace parameter 'struct device' with 'struct cxl_dev_state' in cxl_dvsec_rr_decode(). Remove the unused parameter 'cxl_port' in cxl_dvsec_rr_decode(). [as: massaged commit msg and log] Reviewed-by: Alison Schofield <alison.schofield@intel.com> > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > --- > drivers/cxl/core/pci.c | 6 +++--- > drivers/cxl/cxl.h | 3 ++- > drivers/cxl/port.c | 2 +- > tools/testing/cxl/test/mock.c | 6 +++--- > 4 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index 5b46bc46aaa9..420e4be85a1f 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -291,11 +291,11 @@ static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm) > return devm_add_action_or_reset(host, disable_hdm, cxlhdm); > } > > -int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port, > +int cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds, > struct cxl_endpoint_dvsec_info *info) > { > - struct pci_dev *pdev = to_pci_dev(dev); > - struct cxl_dev_state *cxlds = pci_get_drvdata(pdev); > + struct pci_dev *pdev = to_pci_dev(cxlds->dev); > + struct device *dev = cxlds->dev; > int hdm_count, rc, i, ranges = 0; > int d = cxlds->cxl_dvsec; > u16 cap, ctrl; > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index f6015f24ad38..fdac3ddb8635 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -821,7 +821,8 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port, > int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm, > struct cxl_endpoint_dvsec_info *info); > int devm_cxl_add_passthrough_decoder(struct cxl_port *port); > -int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port, > +struct cxl_dev_state; > +int cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds, > struct cxl_endpoint_dvsec_info *info); > > bool is_cxl_region(struct device *dev); > diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c > index 24041cf85cfb..66e18fe55826 100644 > --- a/drivers/cxl/port.c > +++ b/drivers/cxl/port.c > @@ -98,7 +98,7 @@ static int cxl_endpoint_port_probe(struct cxl_port *port) > struct cxl_port *root; > int rc; > > - rc = cxl_dvsec_rr_decode(cxlds->dev, port, &info); > + rc = cxl_dvsec_rr_decode(cxlds, &info); > if (rc < 0) > return rc; > > diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c > index f4ce96cc11d4..4f82716cfc16 100644 > --- a/tools/testing/cxl/test/mock.c > +++ b/tools/testing/cxl/test/mock.c > @@ -228,16 +228,16 @@ int __wrap_cxl_hdm_decode_init(struct cxl_dev_state *cxlds, > } > EXPORT_SYMBOL_NS_GPL(__wrap_cxl_hdm_decode_init, CXL); > > -int __wrap_cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port, > +int __wrap_cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds, > struct cxl_endpoint_dvsec_info *info) > { > int rc = 0, index; > struct cxl_mock_ops *ops = get_cxl_mock_ops(&index); > > - if (ops && ops->is_mock_dev(dev)) > + if (ops && ops->is_mock_dev(cxlds->dev)) > rc = 0; > else > - rc = cxl_dvsec_rr_decode(dev, port, info); > + rc = cxl_dvsec_rr_decode(cxlds, info); > put_cxl_mock_ops(index); > > return rc; > -- > 2.17.1 > >
On 12/12/24 2:46 PM, Alison Schofield wrote: > On Tue, Dec 03, 2024 at 04:21:12PM +0000, alucerop@amd.com wrote: >> From: Alejandro Lucero <alucerop@amd.com> >> >> CXL Type3 pci driver uses struct device driver_data for keeping >> cxl_dev_state reference. Type1/2 drivers are not only about CXL so this >> field should not be used when code requires cxl_dev_state to work with >> and such a code used for Type2 support. >> >> Change cxl_dvsec_rr_decode for passing cxl_dev_state as a parameter. >> >> Seize the change for removing the unused cxl_port param. >> > > Dave, > > Jonathan previously offered Reviewed-by tag so this looks good to > apply. I've added another review tag and also massaged the > changelog for your consideration. > > cxl/pci: Add Type 1/2 support to cxl_dvsec_rr_decode() > > In cxl_dvsec_rr_decode() the pci driver expects to retrieve a cxlds, > struct cxl_dev_state, from the driver_data field of struct device. > While that works for Type 3, drivers for Type 1/2 devices may not > put a cxlds in the driver_data field. > > In preparation for supporting Type 1/2 devices, replace parameter > 'struct device' with 'struct cxl_dev_state' in cxl_dvsec_rr_decode(). > > Remove the unused parameter 'cxl_port' in cxl_dvsec_rr_decode(). > > [as: massaged commit msg and log] > Reviewed-by: Alison Schofield <alison.schofield@intel.com> Thanks! I'll take it with the commit log changes. > > >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> --- >> drivers/cxl/core/pci.c | 6 +++--- >> drivers/cxl/cxl.h | 3 ++- >> drivers/cxl/port.c | 2 +- >> tools/testing/cxl/test/mock.c | 6 +++--- >> 4 files changed, 9 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c >> index 5b46bc46aaa9..420e4be85a1f 100644 >> --- a/drivers/cxl/core/pci.c >> +++ b/drivers/cxl/core/pci.c >> @@ -291,11 +291,11 @@ static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm) >> return devm_add_action_or_reset(host, disable_hdm, cxlhdm); >> } >> >> -int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port, >> +int cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds, >> struct cxl_endpoint_dvsec_info *info) >> { >> - struct pci_dev *pdev = to_pci_dev(dev); >> - struct cxl_dev_state *cxlds = pci_get_drvdata(pdev); >> + struct pci_dev *pdev = to_pci_dev(cxlds->dev); >> + struct device *dev = cxlds->dev; >> int hdm_count, rc, i, ranges = 0; >> int d = cxlds->cxl_dvsec; >> u16 cap, ctrl; >> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h >> index f6015f24ad38..fdac3ddb8635 100644 >> --- a/drivers/cxl/cxl.h >> +++ b/drivers/cxl/cxl.h >> @@ -821,7 +821,8 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port, >> int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm, >> struct cxl_endpoint_dvsec_info *info); >> int devm_cxl_add_passthrough_decoder(struct cxl_port *port); >> -int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port, >> +struct cxl_dev_state; >> +int cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds, >> struct cxl_endpoint_dvsec_info *info); >> >> bool is_cxl_region(struct device *dev); >> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c >> index 24041cf85cfb..66e18fe55826 100644 >> --- a/drivers/cxl/port.c >> +++ b/drivers/cxl/port.c >> @@ -98,7 +98,7 @@ static int cxl_endpoint_port_probe(struct cxl_port *port) >> struct cxl_port *root; >> int rc; >> >> - rc = cxl_dvsec_rr_decode(cxlds->dev, port, &info); >> + rc = cxl_dvsec_rr_decode(cxlds, &info); >> if (rc < 0) >> return rc; >> >> diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c >> index f4ce96cc11d4..4f82716cfc16 100644 >> --- a/tools/testing/cxl/test/mock.c >> +++ b/tools/testing/cxl/test/mock.c >> @@ -228,16 +228,16 @@ int __wrap_cxl_hdm_decode_init(struct cxl_dev_state *cxlds, >> } >> EXPORT_SYMBOL_NS_GPL(__wrap_cxl_hdm_decode_init, CXL); >> >> -int __wrap_cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port, >> +int __wrap_cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds, >> struct cxl_endpoint_dvsec_info *info) >> { >> int rc = 0, index; >> struct cxl_mock_ops *ops = get_cxl_mock_ops(&index); >> >> - if (ops && ops->is_mock_dev(dev)) >> + if (ops && ops->is_mock_dev(cxlds->dev)) >> rc = 0; >> else >> - rc = cxl_dvsec_rr_decode(dev, port, info); >> + rc = cxl_dvsec_rr_decode(cxlds, info); >> put_cxl_mock_ops(index); >> >> return rc; >> -- >> 2.17.1 >> >>
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index 5b46bc46aaa9..420e4be85a1f 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -291,11 +291,11 @@ static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm) return devm_add_action_or_reset(host, disable_hdm, cxlhdm); } -int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port, +int cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds, struct cxl_endpoint_dvsec_info *info) { - struct pci_dev *pdev = to_pci_dev(dev); - struct cxl_dev_state *cxlds = pci_get_drvdata(pdev); + struct pci_dev *pdev = to_pci_dev(cxlds->dev); + struct device *dev = cxlds->dev; int hdm_count, rc, i, ranges = 0; int d = cxlds->cxl_dvsec; u16 cap, ctrl; diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index f6015f24ad38..fdac3ddb8635 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -821,7 +821,8 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port, int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm, struct cxl_endpoint_dvsec_info *info); int devm_cxl_add_passthrough_decoder(struct cxl_port *port); -int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port, +struct cxl_dev_state; +int cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds, struct cxl_endpoint_dvsec_info *info); bool is_cxl_region(struct device *dev); diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c index 24041cf85cfb..66e18fe55826 100644 --- a/drivers/cxl/port.c +++ b/drivers/cxl/port.c @@ -98,7 +98,7 @@ static int cxl_endpoint_port_probe(struct cxl_port *port) struct cxl_port *root; int rc; - rc = cxl_dvsec_rr_decode(cxlds->dev, port, &info); + rc = cxl_dvsec_rr_decode(cxlds, &info); if (rc < 0) return rc; diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c index f4ce96cc11d4..4f82716cfc16 100644 --- a/tools/testing/cxl/test/mock.c +++ b/tools/testing/cxl/test/mock.c @@ -228,16 +228,16 @@ int __wrap_cxl_hdm_decode_init(struct cxl_dev_state *cxlds, } EXPORT_SYMBOL_NS_GPL(__wrap_cxl_hdm_decode_init, CXL); -int __wrap_cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port, +int __wrap_cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds, struct cxl_endpoint_dvsec_info *info) { int rc = 0, index; struct cxl_mock_ops *ops = get_cxl_mock_ops(&index); - if (ops && ops->is_mock_dev(dev)) + if (ops && ops->is_mock_dev(cxlds->dev)) rc = 0; else - rc = cxl_dvsec_rr_decode(dev, port, info); + rc = cxl_dvsec_rr_decode(cxlds, info); put_cxl_mock_ops(index); return rc;