Re: [PATCH v5 4/6] cxl/acpi: Add downstream port data to cxl_port instances

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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...



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux