linux-kernel - Re: [PATCH v8 07/28] mailbox: Allow direct registration to a channel

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: <8c819213-b820-0ff3-b2c4-d9af1b6b3f83@linaro.org>
Date:   Tue, 17 Jan 2023 13:21:08 -0600
From:   Alex Elder <elder@...aro.org>
To:     Elliot Berman <quic_eberman@...cinc.com>,
        Alex Elder <elder@...aro.org>,
        Bjorn Andersson <quic_bjorande@...cinc.com>,
        Jassi Brar <jassisinghbrar@...il.com>,
        Sudeep Holla <sudeep.holla@....com>
Cc:     Murali Nalajala <quic_mnalajal@...cinc.com>,
        Trilok Soni <quic_tsoni@...cinc.com>,
        Srivatsa Vaddagiri <quic_svaddagi@...cinc.com>,
        Carl van Schaik <quic_cvanscha@...cinc.com>,
        Prakruthi Deepak Heragu <quic_pheragu@...cinc.com>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jonathan Corbet <corbet@....net>,
        Bagas Sanjaya <bagasdotme@...il.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-acpi@...r.kernel.org
Subject: Re: [PATCH v8 07/28] mailbox: Allow direct registration to a channel

On 1/10/23 11:57 AM, Elliot Berman wrote:
> 
> 
> On 1/9/2023 1:34 PM, Alex Elder wrote:
>> On 12/19/22 4:58 PM, Elliot Berman wrote:
>>> Support virtual mailbox controllers and clients which are not platform
>>> devices or come from the devicetree by allowing them to match client to
>>> channel via some other mechanism.
>>
>> The new function behaves very much like mbox_request_channel()
>> did before.
>>
>> The new function differs from omap_mbox_request_channel() in that
>> it can change the if chan->txdone_method is TXDONE_BY_POLL, it
>> is changed to TXDONE_BY_ACK if the client's knows_txdone field is
>> set.  Is this OK?  Why?

Sorry, reading that now, I see I placed an "if" in the wrong spot.

> Both of the current drivers that use mbox_bind_client use TXDONE_BY_IRQ, 
> so this doesn't cause issue for checking whether the client has 
> txdone_method.

I'm not so sure, but it's on you to make sure you don't break
anything...  I see only two spots where TXDONE_BY_IRQ is set,
and TXDONE_BY_IRQ seems to be set when channels are freed.

I spent (too much) time trying to track this back but I'm
giving up.  If you're sure it's correct, I accept that...

>>
>> It also assumes chan->mbox->ops->startup us non-null (though that
>> isn't really a problem).
>>
>>>
>>> Signed-off-by: Elliot Berman <quic_eberman@...cinc.com>
>>> ---
>>>   drivers/mailbox/mailbox.c      | 96 ++++++++++++++++++++++++----------
>>>   drivers/mailbox/omap-mailbox.c | 18 ++-----
>>>   drivers/mailbox/pcc.c          | 18 ++-----
>>>   include/linux/mailbox_client.h |  1 +
>>>   4 files changed, 76 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>>> index 4229b9b5da98..adf36c05fa43 100644
>>> --- a/drivers/mailbox/mailbox.c
>>> +++ b/drivers/mailbox/mailbox.c
>>> @@ -317,6 +317,71 @@ int mbox_flush(struct mbox_chan *chan, unsigned 
>>> long timeout)
>>>   }
>>>   EXPORT_SYMBOL_GPL(mbox_flush);
>>> +static int __mbox_bind_client(struct mbox_chan *chan, struct 
>>> mbox_client *cl)
>>
>> There should be an unbind_client() call.  At a minimum, you are
>> calling try_module_get(), and the matching module_put() call
>> would belong there.  And even though one might just call
>> module_put() elsewhere for this, it would be cleaner to have
>> a function that similarly encapsulates the shutdown call
>> as well.
> n
> The function for this is "mbox_free_channel".

My point is about the way you are abstracting the "bind" operation
as a (now encapsulated) part of requesting the channel.  Yes, when
mbox_free_channel() is called, it effectively "unbinds" the channel.
But you're creating a "bind" abstraction, where it's not explicit
that you're requesting the channel.  I'm suggesting you also create
an "unbind" operation to reverse that.

This is more important for the mbox_bind_client() call than 
mbox_request_channel().  (And by the way, it looks like
pcc_mbox_free_channel() doesn't call pcc_mbox_free_channel()
as it should, but this unfamiliar code...)

And... it's weird to me that gh_rm_drv_probe() calls gh_msgq_init()
(to initialize the Gunhah message queue code), but then has to
call mbox_free_channel() *separate* from gh_msgq_remove(); the
free channel (or better, unbind client) should happen in the
message queue code.

It's not a critically important point, but now at least I hope
you understand what I'm trying to say.

					-Alex
> 
> Thanks,
> Elliot