Re: Sashiko review feedback (was Re: [PATCH 2/3] soc: renesas: Add Renesas R-Car MFIS driver)

[Wolfram Sang]

Hi Geert,

> While there are no overlapping bits, and thus both OR and PLUS have
> the same effect, I agree PLUS is More Correct(TM), as that matches
> what you are using in the second iowrite32() below.

Yes, it is more correct, yes I will change it, yes I don't like
bike-shedding ;)

> > Yes, it can. It even needs a hwspinlock instead of a regular spinlock
> > because it needs protection against cores which may not run Linux.
> > hwspinlock support will only be added later because of dependency
> > issues. My plan was to add the protection once we got hwspinlocks. But
> > maybe I should add just spinlocks now and convert them to hwspinlocks
> > once their support is in.
> 
> Have you ever triggered this race condition when running the mbox
> test? Or is that impossible due to locking elsewhere?

Haven't encountered that yet. But since we are talking about generic
mfis_write() here which can be used in further ways in the future, we
should not rely on external serialization here IMO.

> > Yes, this was overlooked so far.
> 
> .suppress_bind_attrs to the rescue? ;-)

I was considering this for sure, on top of the suggested improvement. I
guess it can be argued for a mailbox and hwspinlock driver, or?

> That is an interesting one I never considered before!
> So far we always assumed of_device_get_match_data() cannot return
> NULL if all match table entries have their .data members filled in.
> However, you can indeed still trigger this by using driver_override
> to bind to the wrong device, which obviously has no match entry at all:

...

> Do we care about that?

I tend to say: yes. On the one hand, using "driver_override" is a
you-better-know-what-you-do command. On the other hand, failing
gracefully is not hard to do and not much code.

Happy hacking,

   Wolfram

Attachment: signature.asc
Description: PGP signature