Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <5d96e40a.1c69fb81.5a60f.fd3a@mx.google.com>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20191004055057.GH63675@minitux>
References: <1570146710-13503-1-git-send-email-mnalajal@codeaurora.org> <5d96daca.1c69fb81.fe5e4.e623@mx.google.com> <20191004055057.GH63675@minitux>
From:   Stephen Boyd <swboyd@chromium.org>
To:     Bjorn Andersson <bjorn.andersson@linaro.org>
Cc:     Murali Nalajala <mnalajal@codeaurora.org>,
        gregkh@linuxfoundation.org, rafael@kernel.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] base: soc: Handle custom soc information sysfs entries
User-Agent: alot/0.8.1
Date:   Thu, 03 Oct 2019 23:17:45 -0700
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Bjorn Andersson (2019-10-03 22:50:57)
> On Thu 03 Oct 22:38 PDT 2019, Stephen Boyd wrote:
>=20
> > Quoting Murali Nalajala (2019-10-03 16:51:50)
> > > @@ -151,14 +156,16 @@ struct soc_device *soc_device_register(struct s=
oc_device_attribute *soc_dev_attr
> > > =20
> > >         ret =3D device_register(&soc_dev->dev);
> > >         if (ret)
> > > -               goto out3;
> > > +               goto out4;
> > > =20
> > >         return soc_dev;
> > > =20
> > > -out3:
> > > +out4:
> > >         ida_simple_remove(&soc_ida, soc_dev->soc_dev_num);
> > >         put_device(&soc_dev->dev);
> > >         soc_dev =3D NULL;
> > > +out3:
> > > +       kfree(soc_attr_groups);
> >=20
> > This code is tricky. put_device(&soc_dev->dev) will call soc_release()
> > so we set soc_dev to NULL before calling kfree() on the error path. This
> > way we don't doubly free a pointer that the release function will take
> > care of. I wonder if the release function could free the ida as well,
> > and then we could just make the device_register() failure path call
> > put_device() and return ERR_PTR(ret) directly. Then the error path is
> > simpler because we can avoid changing two pointers to NULL to avoid the
> > double free twice. Or just inline the ida remove and put_device() call
> > in the if and then goto out1 to consolidate the error pointer
> > conversion.
> >=20
>=20
> But if we instead allocates the ida before the soc_dev, wouldn't the
> error path be something like?:
>=20
> foo:
>         put_device(&soc_dev->dev);
> bar:
>         ida_simple_remove(&soc_ida, soc_num);
>         return err;
>=20
>=20
> I think we still need two exit paths from soc_device_register()
> regardless of moving the ida_simple_remove() into the release, but we
> could drop it from the unregister(). So not sure if this is cleaner...
>=20

It doesn't seem "safe" to let the number be reused before the device is
destroyed by put_device(). It would be clearer to do all the cleanup
from the release function so that the soc_device_unregister() path isn't
racy with another device being registered and reusing the same ID.

Of course this probably doesn't matter because the race I'm talking
about is extremely unlikely given there's only ever one soc device.
Reordering the put and remove would be fine too.