linux-kernel - Re: [PATCH resend] serial: 8250: Add support for using platform

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: <20190521170350.GL9224@smile.fi.intel.com>
Date:   Tue, 21 May 2019 20:03:50 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Esben Haabendal <esben@...bendal.dk>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-serial@...r.kernel.org, Lee Jones <lee.jones@...aro.org>,
        Enrico Weigelt <lkml@...ux.net>, Jiri Slaby <jslaby@...e.com>,
        Darwin Dingel <darwin.dingel@...iedtelesis.co.nz>,
        Jisheng Zhang <Jisheng.Zhang@...aptics.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        He Zhe <zhe.he@...driver.com>, Marek Vasut <marex@...x.de>,
        Douglas Anderson <dianders@...omium.org>,
        Paul Burton <paul.burton@...s.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH resend] serial: 8250: Add support for using
 platform_device resources

On Tue, May 21, 2019 at 04:43:18PM +0200, Esben Haabendal wrote:
> Andy Shevchenko <andriy.shevchenko@...ux.intel.com> writes:
> 
> > On Tue, May 21, 2019 at 01:34:26PM +0200, Esben Haabendal wrote:
> >> Allow getting memory resource (mapbase or iobase) as well as irq from
> >> platform_device resources.
> >> 
> >> The UPF_DEV_RESOURCES flag must be set for devices where platform_device
> >> resources are to be used.  When not set, driver behaves as before.
> >> 
> >> This allows use of the serial8250 driver together with devices with
> >> resources added by platform_device_add_resources(), such as mfd child
> >> devices added with mfd_add_devices().
> >> 
> >> When UPF_DEV_RESOURCES flag is set, the following platform_data fields should
> >> not be used: mapbase, iobase, mapsize, and irq.  They are superseded by the
> >> resources attached to the device.
> >> 
> >
> > Same comment here: Requesting resource is orthogonal to the retrieving or
> > slicing them.
> 
> Yes.  But for MFD devices, I do think it makes sense for the MFD parent
> device to request the entire memory resource, and then split it.

Nope. This is layering violation here: The user of the resources is not
handling them in full.

> And for drivers that actually are aware of the struct resource given,
> both approaches work.  Throwing away the resource.parent information
> and calling out request_mem_region() manually breaks the idea of
> managing IORESOURCE_MEM as a tree structure.

How come? Can you show an example of output without and with your patches?

> Are we not supposed to be using the parent/child part of struct
> resource?

It's about slicing, no-one prevents you to do that. I don't see a problem.
Show the output!

> >> -		if (!request_mem_region(port->mapbase, size, "serial")) {
> >> +		if (!(port->flags & UPF_DEV_RESOURCES) &&
> >> +		    !request_mem_region(port->mapbase, size, "serial")) {
> >
> >> -				release_mem_region(port->mapbase, size);
> >> +				if (!(port->flags & UPF_DEV_RESOURCES))
> >> +					release_mem_region(port->mapbase, size);
> >
> >> -		if (!request_region(port->iobase, size, "serial"))
> >> +		if (!(port->flags & UPF_DEV_RESOURCES) &&
> >> +		    !request_region(port->iobase, size, "serial"))
> >
> >> -		release_mem_region(port->mapbase, size);
> >> +		if (!(port->flags & UPF_DEV_RESOURCES))
> >> +			release_mem_region(port->mapbase, size);
> >
> >> -		release_region(port->iobase, size);
> >> +		if (!(port->flags & UPF_DEV_RESOURCES))
> >> +			release_region(port->iobase, size);
> >
> > All these changes are not related to what you describe in the commit message.
> > is a workaround for the bug in the parent MFD driver of the 8250.
> 
> You are right, this is not adequately described in commit message.
> But unless we are not supposed to allow parent/child memory resource
> management, I don't think it is a workaround, but a fix.
> 
> But I can split it out in a separate patch.  Would be nice if I at least
> can get the other part of the change merged.

Like Lee said, and I agree, nothing prevents us to switch to
platform_get_resource().

The stumbling block here is the *requesting* in parent which I strongly
disagree with (at least in a form of this change, I already told you, that this
has to be "fixed" on generic level, not as a hack in one certain driver).

-- 
With Best Regards,
Andy Shevchenko