Re: [PATCH 2/2] misc: sram: enable clock before registering regions

[Johan Hovold]

On Tue, Jul 03, 2018 at 03:30:09PM +0300, Vladimir Zapolskiy wrote:
> On 07/03/2018 02:47 PM, Johan Hovold wrote:
> > On Tue, Jul 03, 2018 at 01:23:30PM +0300, Vladimir Zapolskiy wrote:
> >> Hi Johan,
> >>
> >> On 07/03/2018 01:05 PM, Johan Hovold wrote:
> >>> Make sure to enable the clock before registering regions and exporting
> >>> partitions to user space at which point we must be prepared for I/O.
> >>>
> >>> Fixes: ee895ccdf776 ("misc: sram: fix enabled clock leak on error path")
> >>> Cc: Vladimir Zapolskiy <vladimir_zapolskiy@xxxxxxxxxx>
> >>> Signed-off-by: Johan Hovold <johan@xxxxxxxxxx>
> >>
> >> thank you for the change, however please note that the identified commit
> >> for the fix is incorrect one apparently.
> >>
> >> In my opinion the proper tag contents would be
> >>
> >> Fixes: b4c3fcb3c71f ("misc: sram: extend usage of reserved partitions")
> >>
> >> I hope you agree to it, also I would suggest to swap the changes in
> >> the series.
> > 
> > No, I think I used the right commit in the Fixes tag as that was the
> > commit which moved the clock enable to after the memory-region
> > registration (at which point the memory could potentially be accessed).
> 
> I was confused by the moved sram_reserve_regions() call, which was added
> way later.
> 
> Allright, if it is assumed that gen_pool_get() interface requires only
> a registered memory pool provider device, and it does, then there is
> another kind of a problem, a SRAM/genpool consumer may not get access
> to a valid region in SRAM before the latter is added to the SRAM pool
> in sram_probe().

Right, this whole genpool interface is fragile, but that's a different
story.

> Instantly I don't know how to solve the issue above, it may require
> a change to lib/genalloc.c to request a registration of genpool device
> driver, but then such a change solves the problem identified by you
> as well.

The resource (genpool) should not be registered before it's been fully
initialised, while any prior attempts to look it up could cause the
consumer driver to defer their probes, for example. But again, that's
beyond the scope here.

> For your change as a proper (partial?) fix:
> 
> Reviewed-by: Vladimir Zapolskiy <vladimir_zapolskiy@xxxxxxxxxx>

Thanks for the review!

Johan