Re: [PATCH v2 1/3] gpio: introduce utilities for synchronous fake device creation

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Thu, Feb 20, 2025 at 2:07 PM Koichiro Den <koichiro.den@xxxxxxxxxxxxx> wrote:
>
> On Thu, Feb 20, 2025 at 12:06:33PM GMT, Bartosz Golaszewski wrote:
> > On Tue, Feb 18, 2025 at 5:04 PM Koichiro Den <koichiro.den@xxxxxxxxxxxxx> wrote:
> > >
> > > Both gpio-sim and gpio-virtuser share a mechanism to instantiate a
> > > platform device, wait for probe completion, and retrieve the probe
> > > success or error status synchronously. With gpio-aggregator planned to
> > > adopt this approach for its configfs interface, it's time to factor
> > > out the common code.

[snip]

> > > +void dev_sync_probe_init(struct dev_sync_probe_data *data)
> > > +{
> > > +       memset(data, 0, sizeof(*data));
> > > +       init_completion(&data->probe_completion);
> > > +       data->bus_notifier.notifier_call = dev_sync_probe_notifier_call;
> > > +}
> > > +EXPORT_SYMBOL_GPL(dev_sync_probe_init);
> > > +
> > > +int dev_sync_probe_register(struct dev_sync_probe_data *data,
> > > +                           struct platform_device_info *pdevinfo)
> > > +{
> > > +       struct platform_device *pdev;
> > > +       char *name;
> > > +
> > > +       name = kasprintf(GFP_KERNEL, "%s.%u", pdevinfo->name, pdevinfo->id);
> >
> > pdevinfo->id is a signed integer
> >
> > I'm also wondering if we could avoid the allocation here and keep on
> > using snprintf() like in the existing drivers? On the other hand,
> > memory is cheap so no big deal.
>
> Are you assuming the following change?
>
>    struct dev_sync_probe_data {
>           struct platform_device *pdev;
>   -       const char *name;
>   +       char name[32];
>
>           /* Synchronize with probe */
>           struct notifier_block bus_notifier;
>

No, I was thinking about a local buffer in the notifier handler, like
what we do currently in gpio-sim, but no worries, you can keep it this
way.

> >
> > > +       if (!name)
> > > +               return -ENOMEM;
> > > +
> > > +       data->driver_bound = false;
> > > +       data->name = name;
> > > +       reinit_completion(&data->probe_completion);
> > > +       bus_register_notifier(&platform_bus_type, &data->bus_notifier);
> > > +
> > > +       pdev = platform_device_register_full(pdevinfo);
> > > +       if (IS_ERR(pdev)) {
> > > +               bus_unregister_notifier(&platform_bus_type, &data->bus_notifier);
> > > +               kfree(data->name);
> >
> > We could probably simplify it by using __free(kfree) with the name
> > variable and just setting it at the end with no_free_ptr().
>
> platform_device_register_full() call path might finish probe so before
> calling it, we need to make sure the 'name' is filled in. That's why I
> didn't used __free(kfree).
>

Not sure I understand this. Would you mind rephrasing?

Bart