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

[Koichiro Den]

On Thu, Feb 20, 2025 at 02:13:42PM GMT, Bartosz Golaszewski wrote:
> 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?

dev_sync_probe_notifier_call() references dev_sync_probe_data's 'name'
field. In dev_sync_probe_register(), platform_device_register_full()
invocation can possibly succeed in the initial probe, meaning that
dev_sync_probe_notifier_call() can be invoked before
platform_device_register_full() returns. So, 'name' field must be set
beforehand, and I located 'data->name = name' as shown above;

If I used __free(kfree), the number of which I needed to write
'no_free_ptr(data->name);' would be the same (= 2 times). So I thought that
calling kfree(data->name) without __free(kfree) was simpler and better.

> 
> Bart