| Message ID | 20241001005234.61409-3-Smita.KoralahalliChannabasappa@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | acpi/ghes, cper, cxl: Trace FW-First CXL Protocol Errors | expand |
Smita Koralahalli wrote: > Refactor computation of cxlds to a common function get_cxl_devstate(). > > The above function could then be reused in both FW-First Component and > Protocol error reporting and handling. > > Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> > --- > v2: > Refactor before adding trace support. > get_cxl_dev() -> get_cxl_devstate(). > Cleaned up get_cxl_devstate(). > --- > drivers/cxl/pci.c | 32 +++++++++++++++++++------------- > 1 file changed, 19 insertions(+), 13 deletions(-) > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 37164174b5fb..915102f5113f 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -1021,32 +1021,38 @@ static struct pci_driver cxl_pci_driver = { > }, > }; > > +static struct cxl_dev_state *get_cxl_devstate(u16 segment, u8 bus, > + u8 device, u8 function) > +{ > + unsigned int devfn = PCI_DEVFN(device, function); > + struct pci_dev *pdev __free(pci_dev_put) = > + pci_get_domain_bus_and_slot(segment, bus, devfn); This is a good cleanup. The previous code should not have declared pdev NULL. However... > + > + if (!pdev) > + return NULL; > + > + guard(device)(&pdev->dev); > + if (pdev->driver != &cxl_pci_driver) > + return NULL; > + > + return pci_get_drvdata(pdev); The device lock is now dropped before the tracing is completed. For this simple code I'm not keen on having the lock be taken in the helper and released later. It seems best to just open code this in each caller. Ira
Smita Koralahalli wrote: > Refactor computation of cxlds to a common function get_cxl_devstate(). > > The above function could then be reused in both FW-First Component and > Protocol error reporting and handling. Ira caught the bug in the cleanup conversion, but I am otherwise not convinced there would be much reuse for a helper like this given any endpoint-flagged protocol errors would reuse the common cxl_handle_cper_event(), unless I am missing something?
On 10/2/2024 4:04 PM, Dan Williams wrote: > Smita Koralahalli wrote: >> Refactor computation of cxlds to a common function get_cxl_devstate(). >> >> The above function could then be reused in both FW-First Component and >> Protocol error reporting and handling. > > Ira caught the bug in the cleanup conversion, but I am otherwise not > convinced there would be much reuse for a helper like this given any > endpoint-flagged protocol errors would reuse the common > cxl_handle_cper_event(), unless I am missing something? > Yeah, the only intent was to not redo the BDF decoding at two places. I don't see a problem in calling cxl_handle_prot_err() inside cxl_handle_cper_event() as Ira suggested in 4/4. However, I think all of this would fall off if we isolate protocol errors from cxl_cper_fifo and handle them separately. Thanks Smita
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 37164174b5fb..915102f5113f 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -1021,32 +1021,38 @@ static struct pci_driver cxl_pci_driver = { }, }; +static struct cxl_dev_state *get_cxl_devstate(u16 segment, u8 bus, + u8 device, u8 function) +{ + unsigned int devfn = PCI_DEVFN(device, function); + struct pci_dev *pdev __free(pci_dev_put) = + pci_get_domain_bus_and_slot(segment, bus, devfn); + + if (!pdev) + return NULL; + + guard(device)(&pdev->dev); + if (pdev->driver != &cxl_pci_driver) + return NULL; + + return pci_get_drvdata(pdev); +} + #define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0) static void cxl_handle_cper_event(enum cxl_event_type ev_type, struct cxl_cper_event_rec *rec) { struct cper_cxl_event_devid *device_id = &rec->hdr.device_id; - struct pci_dev *pdev __free(pci_dev_put) = NULL; enum cxl_event_log_type log_type; struct cxl_dev_state *cxlds; - unsigned int devfn; u32 hdr_flags; pr_debug("CPER event %d for device %u:%u:%u.%u\n", ev_type, device_id->segment_num, device_id->bus_num, device_id->device_num, device_id->func_num); - devfn = PCI_DEVFN(device_id->device_num, device_id->func_num); - pdev = pci_get_domain_bus_and_slot(device_id->segment_num, - device_id->bus_num, devfn); - if (!pdev) - return; - - guard(device)(&pdev->dev); - if (pdev->driver != &cxl_pci_driver) - return; - - cxlds = pci_get_drvdata(pdev); + cxlds = get_cxl_devstate(device_id->segment_num, device_id->bus_num, + device_id->device_num, device_id->func_num); if (!cxlds) return;
Refactor computation of cxlds to a common function get_cxl_devstate(). The above function could then be reused in both FW-First Component and Protocol error reporting and handling. Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> --- v2: Refactor before adding trace support. get_cxl_dev() -> get_cxl_devstate(). Cleaned up get_cxl_devstate(). --- drivers/cxl/pci.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)