Re: [PATCH v2 1/6] platform/chrome: cros_ec: fix error handling in cros_ec_register()

[Tzung-Bi Shih <tzungbi@google.com>]

On Tue, Feb 08, 2022 at 09:38:55PM -0800, Prashant Malani wrote:
> On Tue, Feb 8, 2022 at 8:50 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
> >
> > Fix cros_ec_register() doesn't unregister platform devices if
> > blocking_notifier_chain_register() fails.
> 
> This isn't grammatically correct. Instead:
> Fix cros_ec_register() to unregister platform devices if blocking....

Thanks, will fix in next version.

> > Also use the single exit path to handle the platform device
> > unregistration.
> >
> > This fix depends on the fact that all the callers of cros_ec_register()
> > allocate zeroed memory.
> 
> Is that a fair assumption? What happens if a future driver calls
> cros_ec_register()
> without zeroed memory?

Suppose ec_dev->pd is garbage data, platform_device_unregister() will operate
on invalid memory address.

Let's use another guard condition in next version instead of relying on
zeroed memory.

> > ---
> What has changed since v2? Please add a change log of the versions here.

They are in the cover letter actually.  Will copy them to each patch.

> > @@ -291,6 +289,12 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> >                 cros_ec_irq_thread(0, ec_dev);
> >
> >         return 0;
> > +exit:
> > +       if (!IS_ERR_OR_NULL(ec_dev->pd))
> > +               platform_device_unregister(ec_dev->pd);
> > +       if (!IS_ERR_OR_NULL(ec_dev->ec))
> > +               platform_device_unregister(ec_dev->ec);
> > +       return err;
> You don't need these "if" checks. They are already performed by
> platform_device_unregister(), or rather, the functions that it calls [1][2].

Ack, will remove the check in next version.