linux-kernel - Re: [PATCH 1/2] serial: 8250: Allow port registration without UPF_BOOT

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: <87v9ywhors.fsf@haabendal.dk>
Date:   Mon, 29 Apr 2019 16:25:27 +0200
From:   Esben Haabendal <esben@...bendal.dk>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     "open list\:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>,
        Darwin Dingel <darwin.dingel@...iedtelesis.co.nz>,
        He Zhe <zhe.he@...driver.com>,
        Jisheng Zhang <Jisheng.Zhang@...aptics.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF

Andy Shevchenko <andy.shevchenko@...il.com> writes:

> On Mon, Apr 29, 2019 at 11:29:05AM +0200, Esben Haabendal wrote:
>> Andy Shevchenko <andy.shevchenko@...il.com> writes:
>> > On Mon, Apr 29, 2019 at 9:27 AM Esben Haabendal <esben@...bendal.dk> wrote:
>> >> Andy Shevchenko <andy.shevchenko@...il.com> writes:
>> >> > On Sat, Apr 27, 2019 at 12:01 PM Esben Haabendal <esben@...bendal.dk>
>> >> > wrote:
>> >> >> Andy Shevchenko <andriy.shevchenko@...ux.intel.com> writes:
>> >> >> > On Fri, Apr 26, 2019 at 06:54:05PM +0200, Esben Haabendal wrote:
>> >> >> >> Andy Shevchenko <andriy.shevchenko@...ux.intel.com> writes:
>
>> So maybe we should go down that direction intead, extending 8250 driver
>> to replace mapbase with a resource struct instead?
>> 
>> > Btw, we have PCI MFD driver which enumerates 8250 (more precisely
>> > 8250_dw) w/o any issues.
>> 
>> I am aware of that (sm501.c).  It avoids the problem by not requesting
>> the parent memory region (sm->io_res), and requesting all child memory
>> regions directly in root instead of relative to the sm->io_res parent.
>> 
>> But as resoure management is designed for managing a parent/child
>> resource tree, this looks much more like a workaround than the right
>> solution.
>> 
>> >> It would be nice if child drivers requesting memory would pass the
>> >> parent memory resource.  Maybe 8250 driver could be changed to accept a
>> >> struct resource pointer instead of a simple mapbase value, allowing to
>> >> setup the resource with parent pointing to the MFD memory resource.
>> >
>> > I don't see the problem in certain driver, I guess you are trying to
>> > workaround existin Linux device resource model.
>> 
>> No, I actually try to do the right thing in relation to Linux device
>> resource model.  But 8250 is just not behaving very well in that
>> respect, not having been made really aware of the resource model.
>
> The point here is that. MFD driver can re-use existing platform drivers which
> may be used standalone. They and only they are the right owners of the
> requesting *their* resources.
>
> When parent request resources on the behalf of its child it simple will break
> this flexibility.

I hear what you say.  And I agree, parent/mfd drivers should not request
resources on behalf of it's children.

But on the other side, something is broken by design in mfd, if it is
not possible for parent/mfd driver to request the resources for itself,
which naturally contains the resources for all it's mfd
functions/childs.

Please take a look at mfd_add_device() in drivers/mfd/mfd-core.c, and
the use of the cell and mem_base arguments in particular.

        for (r = 0; r < cell->num_resources; r++) {
                res[r].name = cell->resources[r].name;
                res[r].flags = cell->resources[r].flags;

                /* Find out base to use */
                if ((cell->resources[r].flags & IORESOURCE_MEM) && mem_base) {
                        res[r].parent = mem_base;
                        res[r].start = mem_base->start +
                                cell->resources[r].start;
                        res[r].end = mem_base->start +
                                cell->resources[r].end;

It really is a core part of mfd drivers that you request a memory
resource for the mfd driver, and then have one or more child memory
resources requsted with the parent memory resource as base.

Many mfd drivers handle resources like this, like: htc/pasic3.c,
sta2x11-mfd.c and intel-lpss.c.
Ofcourse, the sm501.c driver does not, as it does not use the mfd API at
all, and is thus not a good example of a mfd driver, and there is
therefore no good examples of using 8250 with an mfd driver.

/Esben