Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20220118202234.410555-1-terry.bowman@amd.com> <20220118202234.410555-3-terry.bowman@amd.com>
 <CAHp75VdBFN+QMJpYDp8ytGGrBKYyjxU8u=Xrn44Lc3UGLPRQOA@mail.gmail.com> <b4f66a42-6f25-7e93-2a3b-5d73af39e72e@roeck-us.net>
In-Reply-To: <b4f66a42-6f25-7e93-2a3b-5d73af39e72e@roeck-us.net>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Thu, 20 Jan 2022 13:13:03 +0200
Message-ID: <CAHp75VfcJ1HBM=Rw5LcTZWYMxxpASC2b=gTWTrRs4--CmqafpQ@mail.gmail.com>
Subject: Re: [PATCH v3 2/4] Watchdog: sp5100_tco: Refactor MMIO base address initialization
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     Terry Bowman <terry.bowman@amd.com>,
        linux-watchdog@vger.kernel.org, Jean Delvare <jdelvare@suse.com>,
        linux-i2c <linux-i2c@vger.kernel.org>,
        Wolfram Sang <wsa@kernel.org>,
        "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Wim Van Sebroeck <wim@linux-watchdog.org>,
        Robert Richter <rrichter@amd.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        "Shah, Nehal-bakulchandra" <Nehal-bakulchandra.Shah@amd.com>,
        Basavaraj Natikar <Basavaraj.Natikar@amd.com>,
        Shyam Sundar S K <Shyam-sundar.S-k@amd.com>,
        Mario Limonciello <Mario.Limonciello@amd.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jan 19, 2022 at 5:46 PM Guenter Roeck <linux@roeck-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@amd.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