linux-kernel - Re: [PATCH v3 2/4] Watchdog: sp5100_tco: Refactor MMIO base address initialization

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]

Message-ID: <CAHp75VfcJ1HBM=Rw5LcTZWYMxxpASC2b=gTWTrRs4--CmqafpQ@mail.gmail.com>
Date:   Thu, 20 Jan 2022 13:13:03 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Terry Bowman <terry.bowman@....com>,
        linux-watchdog@...r.kernel.org, Jean Delvare <jdelvare@...e.com>,
        linux-i2c <linux-i2c@...r.kernel.org>,
        Wolfram Sang <wsa@...nel.org>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Robert Richter <rrichter@....com>,
        Tom Lendacky <thomas.lendacky@....com>,
        "Shah, Nehal-bakulchandra" <Nehal-bakulchandra.Shah@....com>,
        Basavaraj Natikar <Basavaraj.Natikar@....com>,
        Shyam Sundar S K <Shyam-sundar.S-k@....com>,
        Mario Limonciello <Mario.Limonciello@....com>
Subject: Re: [PATCH v3 2/4] Watchdog: sp5100_tco: Refactor MMIO base address initialization

On Wed, Jan 19, 2022 at 5:46 PM Guenter Roeck <linux@...ck-us.net> wrote:
> On 1/19/22 3:53 AM, Andy Shevchenko wrote:
> > On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <terry.bowman@....com> wrote:

...

> >> +               devm_release_mem_region(dev, mmio_addr,
> >> +                                       SP5100_WDT_MEM_MAP_SIZE);
> >
> > Why? If it's a short live mapping, do not use devm.
>
> This is not short lived; it is needed by the driver. The release
> is an artifact of calling this function twice and ignoring the error
> from devm_ioremap() if the first call fails. devm_release_mem_region()
> isn't strictly needed but that would result in keeping the memory
> region reserved even though it isn't used by the driver.

So, this seems like micro-optimization, but okay, at least it
justifies it. Thanks for explaining.

> There is a functional difference to the original code, though.
> The failing devm_ioremap() causes the code to try the alternate
> address. I am not sure if that really adds value; devm_ioremap()
> fails because the system is out of virtual memory, and calling
> it again on a different address doesn't seem to add much value.
> I preferred the original code, which would only call devm_ioremap()
> after successfully reserving a memory region.

...

> > On top of above it's a NIH devm_ioremap_resource().
>
> Not sure what NIH means

Not Invented Here (syndrome)

...

> >> +       /* Check MMIO address conflict */
> >> +       ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name);
> >
> >> +
> >> +       /* Check alternate MMIO address conflict */
> >
> > Unify this with the previous comment.
>
> Why ? It refers to the code below. If that is a single or two comments
> is really just POV.

It depends on the angle from which you see it (i.o.w. like you said
POV). I considered it from the code perspective and personally found
the
/*
 * Bla bla bla
 */
ret = foo();
if (ret)
  bar();

better than above.

> >> +       if (ret)
> >> +               ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr,
> >> +                                               dev_name);

...

> >>          release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
> >
> > Is it still needed? I have no context to say if devm_iomap() and this
> > are not colliding, please double check the correctness.
> >
> Not sure I understand. This is the release of the io region reserved with
> request_muxed_region() at the beginning of this function. Why would it no
> longer be necessary to release that region ?

Thank you for explaining, as I said I have no full context here, and I
simply asked for double check.

-- 
With Best Regards,
Andy Shevchenko