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

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

On 21-11-04 13:00:35, Dan Williams wrote:
> On Thu, Nov 4, 2021 at 12:46 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > 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()?
> 
> devm_release_action() is not needed if the devm is allowed to fire
> naturally which was the realization I came to here:
> 
> https://lore.kernel.org/r/CAPcyv4gpA=DH0SQvRdmF6dY01mZ1S-gGEWTSDbb+0ajYtyNv0A@mail.gmail.com
> 
> I.e. I regret opening that pandora's box of registering "remote" devm
> actions vs typical local devm actions.

I wasn't suggesting release() fixes anything, but it did seem like we could get
rid of cxl_decoder_autoremove() if we switched it to release(). Must of this
gets too complex for me too quickly though, so if you think it won't work I'm
happy to leave it at that.