On Wed, Jun 9, 2021 at 4:28 AM Jonathan Cameron
<Jonathan.Cameron@xxxxxxxxxx> wrote:
> > > This is a bit inconsistent wrt to what functions get full kernel-doc.
> > > My personal preference would be all the exported functions + any others
> > > where it is particularly useful.
> >
> > I agree with the sentiment for globally exported symbols. In this case
> > they are in the "CXL" module namespace and privately defined in
> > drivers/cxl/ headers. That said, I did document devm_add_cxl_port(),
> > so there's no good reason to skip the documentation on the other
> > devm_cxl_add_* routines... will fix.
>
> Maybe we should consider using symbol namespaces for CXL?
> EXPORT_SYMBOL_NS_GPL() etc
In fact, we already are using that, it's just implicit from the
Makefile with this line:
ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL
> > > > + * Append downstream port data to a cxl_port, note that all allocations
> > > > + * and links are undone by cxl_port deletion and release.
> > > > + */
> > > > +int cxl_add_dport(struct cxl_port *port, struct device *dport_dev, int port_id,
> > > > + resource_size_t component_reg_phys)
> > > > +{
> > > > + char link_name[CXL_TARGET_STRLEN];
> > > > + struct cxl_dport *dport;
> > > > + int rc;
> > > > +
> > > > + if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >=
> > > > + CXL_TARGET_STRLEN)
> > > > + return -EINVAL;
> > > > +
> > > > + dport = kzalloc(sizeof(*dport), GFP_KERNEL);
> > > > + if (!dport)
> > > > + return -ENOMEM;
> > > > +
> > > > + INIT_LIST_HEAD(&dport->list);
> > > > + dport->dport = get_device(dport_dev);
> > > > + dport->port_id = port_id;
> > > > + dport->component_reg_phys = component_reg_phys;
> > > > + dport->port = port;
> > > > +
> > > > + rc = add_dport(port, dport);
> > > > + if (rc)
> > >
> > > If you get an error here, it's not been added to the list, but
> > > in the cxl_dport_release() you remove it from the list. I think you
> > > just want to put and free the device here.
> >
> > The delete is innocuous because of the INIT_LIST_HEAD() above. So the
> > delete will end up doing the right thing and leaving the entry empty
> > again, and that saves the need for custom code to handle that case.
>
> Ah fair enough. I'd missed that INIT. Not sure I'm keen on that
> approach as it's not in the 'obviously correct' category but it's your
> code to maintain so I'm not that fussed.
I think it's in a similar spirit to devm and the lack of
ida_destroy(), try not to write unwind code if at all possible...
