Re: [PATCH v10 12/15] cxl/pci: Disable root port interrupts in RCH mode

[Dan Williams]

Terry Bowman wrote:
> Hi Dan,
> 
> I added comments below.
> 
> On 9/15/23 13:43, Dan Williams wrote:
> > Terry Bowman wrote:
> >> The RCH root port contains root command AER registers that should not be
> >> enabled.[1] Disable these to prevent root port interrupts.
> >>
> >> [1] CXL 3.0 - 12.2.1.1 RCH Downstream Port-detected Errors
> >>
> >> Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>
> >> Signed-off-by: Robert Richter <rrichter@xxxxxxx>
> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >> Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> > [..]
> >> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> >> index 2a22a7ed4704..d195af72ed65 100644
> >> --- a/drivers/cxl/core/port.c
> >> +++ b/drivers/cxl/core/port.c
> >> @@ -1042,6 +1042,9 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> >>  
> >>  	cxl_dport_map_regs(dport);
> >>  
> >> +	if (dport->rch)
> >> +		cxl_disable_rch_root_ints(dport);
> >> +
> > 
> > Similar to the comment about cxl_dport_map_regs() not being appropriate
> > in an enumeration routine, this also needs to move out of _add_dport. It
> > occurs to me that it should also be undone on driver detach just like
> > other device "enables".
> 
> Ok. I will move out of enumeration. 
> 
> Per the 'undo' request: This is a RCH downstream port (dport) with PCIe root port 
> capability. PCI spec states root port error reporting is disabled by default at 
> powerup. And SW does *not* enable the root port errors because the RCH dport is *not* 
> bound to a root port driver (missing BDF, etc). This mask is added to follow the 
> CXL spec precisely and if the rest of the system behaves as expected should not 
> be necessary. 

Ah, got it perhaps add a comment to sanity check that the hardware is in
the per-spec state. Are you certain that even in firmware-first error
handling it is safe for the driver to unconditionally disable these
interrupts?

> I don't believe masking should be 'undone' in driver detach or elsewhere. Adding 
> the 'undo' masking would potentially introduce RCH dport root port interrupt 
> reporting which is incorrect for the RCH/RCD mode. Only CXL components (device, 
> uport, switch) may reside under the RCH dport and never want RCH dport reporting 
> root port errors. RCEC reports the root complex errors in RCH/RCD mode.

Ok, that also seems to suggest that even in the firmware-first case the
driver should make sure they are off per-spec.