Re: [RFC PATCH v2 08/28] cxl/port: Introduce a port driver

[Ben Widawsky <ben.widawsky@intel.com>]

On 21-11-04 12:17:48, Dan Williams wrote:
> On Thu, Nov 4, 2021 at 9:37 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > On 21-10-29 18:37:36, Dan Williams wrote:
> > > On Fri, Oct 22, 2021 at 11:37 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > > >
> >
> > [snip]
> >
> > >
> > > +void trigger_decoder_autoremove(void *data)
> > > +{
> > > +       struct cxl_decoder *cxld = data;
> > > +       struct device *host = get_cxl_topology_host();
> > > +
> > > +       /* The topology host driver beat us to the punch */
> > > +       if (!host)
> > > +               return;
> > > +
> > > +       devm_remove_action(host, cxld_unregister, &cxld->dev);
> > > +       put_cxl_topology_host(host);
> > > +}
> > > +
> > > +/**
> > > + * cxl_port_decoder_autoremove - remove decoders discovered by the port driver
> > > + * @cxld: decoder to remove
> > > + *
> > > + * CXL.mem requires an intact port / decoder topology from root level
> > > + * platform decoders to endpoint decoders. Arrange for decoders
> > > + * enumerated by the port driver to be removed when either the root is
> > > + * removed (in which the entire hierarchy is removed), or when an
> > > + * individual port is disabled, in which case only that port's
> > > + * sub-section of the hierarchy is removed.
> > > + */
> > > +int cxl_port_decoder_autoremove(struct cxl_decoder *cxld)
> > > +{
> > > +       struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> > > +       struct device *host = get_cxl_topology_host();
> > > +       int rc;
> > > +
> > > +       if (!host)
> > > +               return -ENODEV;
> > > +
> > > +       if (!port->dev.driver) {
> > > +               rc = -ENXIO;
> > > +               goto out;
> > > +       }
> > > +
> > > +       rc = cxl_decoder_autoremove(host, cxld);
> > > +       if (rc)
> > > +               goto out;
> > > +
> > > +       rc = devm_add_action_or_reset(&port->dev, trigger_decoder_autoremove,
> > > +                                       cxld);
> > > +out:
> > > +       put_cxl_topology_host(host);
> > > +       return rc;
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(cxl_port_decoder_autoremove, CXL);
> >
> > The only port which has decoders that aren't discovered by the port driver would
> > be the "root port" because it's platform specific. However, that port still is
> > treated similarly to the other ports. Therefore I don't see why you need
> > cxl_decoder_autoremove() anymore. Could you please explain? I think the safety
> > check in trigger_decoder_autoremove() makes this work for all cases.
> 
> I don't think it's worth the games to explain why CXL sees fit to
> register (here comes your favorite argument...) non-idiomatic devm
> actions on devices that are not associated with the running device +
> driver. So as long as the port driver auto-unloads its child devices
> then we're golden. Is your concern that you want to have the CFMWS
> decoders registers from cxl_port and not cxl_acpi?

Hey! I'm all for idioms when it makes sense. To me, one of the coolest things
about working on a new subsystem is you get to define some of your own idioms,
but anyway...

First, no I think the platform driver (cxl_acpi) should enumerate the platform
decoders. I confused devm_remove_action() with devm_release_action(). I wonder
though, if you made trigger_decoder_autoremove() call devm_release_action, would
that be sufficient and then remove cxl_decoder_autoremove()?