linux-kernel - Re: [PATCH 7/8] rtc: rx8010: fix indentation in probe()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 7 Sep 2020 11:34:59 +0200
From:   Bartosz Golaszewski <bgolaszewski@...libre.com>
To:     Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc:     Bartosz Golaszewski <brgl@...ev.pl>,
        Alessandro Zummo <a.zummo@...ertech.it>,
        linux-rtc@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 7/8] rtc: rx8010: fix indentation in probe()

On Fri, Sep 4, 2020 at 5:41 PM Alexandre Belloni
<alexandre.belloni@...tlin.com> wrote:
>
> On 04/09/2020 17:21:15+0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@...libre.com>
> >
> > Align the arguments passed to devm_rtc_device_register() with the upper
> > line.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@...libre.com>
> > ---
> >  drivers/rtc/rtc-rx8010.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/rtc/rtc-rx8010.c b/drivers/rtc/rtc-rx8010.c
> > index 181fc21cefa8..ed8ba38b4991 100644
> > --- a/drivers/rtc/rtc-rx8010.c
> > +++ b/drivers/rtc/rtc-rx8010.c
> > @@ -450,7 +450,7 @@ static int rx8010_probe(struct i2c_client *client,
> >       }
> >
> >       rx8010->rtc = devm_rtc_device_register(&client->dev, client->name,
> > -             &rx8010_rtc_ops, THIS_MODULE);
> > +                                            &rx8010_rtc_ops, THIS_MODULE);
> >
>
> You have bonus points if you replace that patch by switching from
> devm_rtc_device_register to devm_rtc_allocate_device and
> rtc_register_device.
>
> More bonus points if you also set range_min and range_max and then get
> rid of the range checking in set_time.
>

Hi Alexandre!

I've just looked at the code and wondered why there's no devm
counterpart for rtc_register_device(). Then I noticed that the release
callback for devm_rtc_allocate_device() takes care of unregistering
the device. This looks like serious devres abuse to me. In general the
idea is for the release callback to only undo whatever the devres
function did and this should be opaque to the concerned resources.

In this case I believe there's no need for the 'registered' field in
struct rtc_device - this structure should *not* care about this - and
there should be devm_rtc_register_device() whose release callback
would take care of the unregistering. Since this function would be
called after devm_rtc_allocate_device(), it would be released before
so the ordering should be fine.

Let me know your thoughts.

Best regards,
Bartosz Golaszewski