Re: [PATCH v2 2/8] ACPI: property: Parse _CRS CSI-2 descriptor

Andy Shevchenko
2023-01-23T17:25:32Z

On Mon, Jan 23, 2023 at 06:07:43PM +0200, Sakari Ailus wrote:
> On Mon, Jan 23, 2023 at 05:07:09PM +0200, Andy Shevchenko wrote:
> > On Mon, Jan 23, 2023 at 03:46:11PM +0200, Sakari Ailus wrote:

...

> > > + memcpy(inst->remote_name, csi2->resource_source.string_ptr,
> > > + csi2->resource_source.string_length);
> >
> > Why don't we use strscpy()? Is it really strings? Or is it some abuse of
> > the ACPI object type?
>
> I didn't find a guarantee it would be nil terminated. Albeit I'm fine
> switching to strscpy() if there's such a guarantee.

Following this logic neither memcpy() (and especially memcpy()!) call
guarantees that. But hold on, have you actually read strscpy() documentation?

...

> > > + list_replace(&inst_context.res_list, &csi2->buses);
> > > + list_add(&csi2->list, list);
> >
> > Hmm... Can list_swap() be used here?
>
> We're replacing an entry in a list and then adding an entry to another. How
> would you use list_swap() here?

I see, so it is about two different lists.

...

> > > + /*
> > > + * Figure out how much temporary storage we need for counting
> > > + * connections in each device.
> > > + */
> > > + list_for_each_entry(csi2, &crs_csi2_handles, list) {
> > > + struct crs_csi2_instance *inst;
> > > +
> > > + handle_count++;
> >
> > > + list_for_each_entry(inst, &csi2->buses, list)
> > > + handle_count++;
> >
> > list_count_nodes()?
>
> Are you suggesting adding a new list API function or using one that's not
> in the linux-acpi/testing branch yet?

It's in USB tree.
I'm fine if you switch your code later on.

> > > + }

--
With Best Regards,
Andy Shevchenko