Re: [PATCH 01/05] fbdev: sh_mipi_dsi: Remove request/release mem region

[Paul Mundt]

On Wed, Nov 17, 2010 at 05:51:41PM +0900, Magnus Damm wrote:
> On Wed, Nov 17, 2010 at 4:25 PM, Paul Mundt <lethal@xxxxxxxxxxxx> wrote:
> > No. This comes up time and time again for some reason, I'm not sure why
> > this misconception keeps being propagated (and it's definitely a mistake
> > I've made myself too!)
> 
> Perhaps the confusing part is the -EBUSY error case for
> insert_resource() in platform_device_add() located in
> drivers/base/platform. Judging by that code it sure looks like there
> is some kind of conflict management, but probably not enough as you
> mention below.
> 
This is conflict management on a different level, and has nothing to do
with the in-use region case which is separately accounted. It will handle
duplicate insertion of the same resource, but that doesn't prevent
multiple users from getting a handle on the same memory within the
region. request_mem_region() and friends provide this explicitly.

> > The current logic is quite correct in what it is doing.
> 
> It may be correct, but I don't like the smell of it.
> 
> Sound like begging for trouble to me - to require the majority of the
> drivers (the simple ones) to call request/release_mem_region() even
> though by context it's quite obvious that the driver will use
> resource. It doesn't make sense to pass resources that are not going
> to be used, does it?
> 
Begging for trouble is deleting the resource management and hoping for
the best. Plenty of drivers pass in a large resource and then carve it up
internally, it's only a detail the driver can work out for itself, and
this is not knowledge that can be asserted generically.

You seem to have selectively ignored the MFD example that I cited, but
there are plenty of non-MFD cases that do this as well. Some drivers do
follow the one resource per logical abstraction approach that you seem to
favour, mostly using named resources and groveling with
platform_get_resource_byname(), but this is hardly the only way to go
about it, and is by far the exception rather than the rule.

> Also, there are quite a few levels of resource handling / conflict
> checking / device access:
> 
> 1) board/cpu code - contains struct resource for devices
> 2) platform_device_add() - insert_resource() does basic checks
> 3) driver probe() request_mem_region() does more checks, reserves
> 4) driver probe() ioremap() gets a virtual address range
> 5) driver uses ioreadN()/iowriteN() on virtual address range
> 
And each one of these steps is completely orthogonal to the other, so
what exactly is your point? As it is, these all layer quite cleanly.

> The physical range provided by struct resource and the virtual address
> range from ioremap() are of course different things, but i think there
> is a bit of overlap there. The driver isn't supposed to be using
> ioremap() on an I/O memory region that hasn't been locked down with
> request_mem_region() for instance.
> 
> I'd like to see a simplified interface that handles
> request_mem_region() and ioremap() in the driver code, perhaps
> together with automatic handling of request_irq(). Perhaps that's
> difficult in real life, but if possible then that would cut away quite
> a bit of code.
> 
The IRQ thing is obviously wholly unrelated and so doesn't factor in, but
for the memory case you cite it sounds like you're after some devres and
general helpers akin to what the PCI code has going with things like
pci_request_regions() and the managed iomap helpers.

Things work reasonably well in PCI land since you have at least a fairly
logical and consistent abstraction in terms of how the BARs are arranged,
but even then the request/remap helpers take a BAR mask to work out which
ones to deal with in what way (your ioremap()'ed window will want to be
requested, but not all requested windows will want to be remapped). You
could probably do this for platform devices too if you really wanted to,
but you're not likely to be able to generalize much beyond that.

The end result however is that you're still required to deal with
flagging the region (or the parts of the region your driver is
immediately concerned with) as busy, so dropping it as you are doing here
is unacceptable regardless of whether you are using a helper to make life
easier or not.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/