Re: [RFC 2/3] mailbox: Introduce a new common API

[Jassi Brar]

Hello Suman,

On 10 May 2013 05:48, Suman Anna <s-anna@xxxxxx> wrote:

>> No, please. The controller driver should not implement any policy (of
>> allowing/disallowing requests). It should simply try to do as
>> directed. If the client screwed up even after getting info from
>> platform_data/DT, let it suffer.
>
> This would be the same as a client trying to misconfigure a link, you
> cannot blame on a client screw up.
>
One kernel code should not suspect other kernel code's intentions. The
safety/security is in allowing only well written and trusted code
inside the kernel space.
If a client misconfigures a link, the best we could do is call it
buggy. BTW such a hell-bent client would rather screw up with a BUG(),
what do we do then?
So no please, I am not convinced the controller driver should have
anything to do with allow/disallow policy, or any policy at all.

>>>>>
>>>> Already one call ipc_link_ops.startup() reaches the controller upon
>>>> ipc_request_channel.
>>>>
>>>> Do you mean ?
>>>>        ipc_con_ops.startup();
>>>>        ipc_link_ops.startup();
>>>
>>> Yes.
>>>
>> And let the controller startup()/shutdown() upon every
>> link_request/release?
>
> Yes, and the controller driver can take care of any ref-counting or
> whatever other logic it needs to maintain. You see this outside in the
> previous mailbox code, but that is not the responsibility of the core code.
>
The pm_runtime imbalance that you mentioned is the result of OMAP's
API trying to do just that.

>>
>> BTW, I have seen my 2 controllers, OMAP, PL320 and read the dbx500
>> driver. Unsurprisingly none of these have any use for what you call a
>> special ipc_con_ops.startup().  Lets say if the call were there, what
>> would the OMAP put in it?
>
> Enabling the clock for the device. The clock is for the entire IP, a
> link has no power/clock dependencies.
>
OMAP controller driver doesn't do clocks.
It does do rpm though, which does a better job of what OMAP needs.
Please see how pm_runtime_xxx calls are made without worry at each
ipc_link_ops.startup/shutdown.

For controllers that do do clocks, having contiguous
        ipc_con_ops.startup();
        ipc_link_ops.startup();
doesn't save them any complexity. They still have to keep track of
first ipc_link_ops.startup() call in ipc_con_ops.startup() and last
ipc_link_ops.shutdown() call in ipc_con_ops.shutdown()
Which they could very well do in link_ops, if not rpm.

>>
>>>>> Yeah, the pm_runtime_enable cannot be called twice when mailbox is
>>>>> invoked from multiple clients on separate links, so there has to a
>>>>> controller-level logic/ref-counting for that. The clocking for us is on
>>>>> the controller.
>>>>>
>>>> No.  You could call pm_runtime_enable/disable any number of times as
>>>> long as they are balanced. The core does refcounting.
>>>
>>> Exactly, as long as they are balanced. I have two clients dealing with
>>> two remotes (using two links) so pm_runtime_enable on the h/w block
>>> needs to be called only when the first one comes in.
>>>
>> Actually you just gave another reason why the API messing around with
>> controller's power state is a bad idea.
>
> Where do you expect to power up the device (obviously this depends on
> the SoC, and its functional purpose)?
>
Use RPM.  Just like OMAP controller driver already does -
unconditionally call pm_runtime_get upon each link request and
pm_runtime_put upon each link release. Neither the controller nor the
API would have to do refcounting.  If rpm isn't supported, only then
the controller should do refcounting within linp_ops.startup/shutdown
(anyways you want the con_ops and link_ops to be always contiguous).


>> See how mailbox_startup() tries to balance mbox->ops->startup() and
>> mailbox_fini() the mbox->ops->shutdown()  That's very fragile and the
>> cause of imbalance between rpm enable/disable, unless your clients are
>> buggy.
>
> Yeah, it is kinda messed up in the existing code, the startup defined
> there is really for the controller and not the link, and that's why you
> see all the ref-counting balancing logic. The rpm enable/disable being
> called is on the controller's dev, not the link's dev - maybe that's
> what confused you.
>
I wrote the driver for this api, so I do realize the rpm is on
controller. Links are not devices anyways.

cheers,
jassi
--
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/