linux-kernel - Re: [PATCH] mailbox/bcm2835: Fix mailbox full detection.

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: <878uc8pe7t.fsf@eliezer.anholt.net>
Date:	Thu, 28 May 2015 14:46:46 -0700
From:	Eric Anholt <eric@...olt.net>
To:	Stephen Warren <swarren@...dotorg.org>
Cc:	linux-kernel@...r.kernel.org,
	Jassi Brar <jassisinghbrar@...il.com>,
	linux-rpi-kernel@...ts.infradead.org
Subject: Re: [PATCH] mailbox/bcm2835: Fix mailbox full detection.

Stephen Warren <swarren@...dotorg.org> writes:

> On 05/13/2015 02:10 PM, Eric Anholt wrote:
>> With the VC reader blocked and the ARM writing, MAIL0_STA reads empty
>> permanently while MAIL1_STA goes from empty (0x40000000) to non-empty
>> (0x00000001-0x00000007) to full (0x80000008).
>>
>> This bug ended up having no effect on us, because all of our
>> transactions in the client driver were synchronous and under a mutex.
>
> If you could get someone at the RPi Foundation or Broadcom to update the 
> register descriptions and example code at the following URLs, that would 
> be rather useful. Otherwise, this code will appear incorrect when 
> compared against the documentation:
>
> https://github.com/raspberrypi/firmware/wiki/Mailboxes
> ("Mailbox registers" at the bottom)
>
> https://github.com/raspberrypi/firmware/wiki/Accessing-mailboxes
> ("Sample code")

Since it's a wiki, I went ahead and edited the first one.  Hopefully
that clarifies how the c++ in the other page is supposed to be used.

>> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
>
>> @@ -117,7 +118,7 @@ static bool bcm2835_last_tx_done(struct mbox_chan *link)
>>   	bool ret;
>>
>>   	spin_lock(&mbox->lock);
>> -	ret = !(readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL);
>> +	ret = !(readl(mbox->regs + MAIL1_STA) & ARM_MS_FULL);
>
> What does "tx done" mean semantically?
>
> If "tx done" means "remote side received all our messages", then surely 
> this should check MAIL1_STA for emptiness, which is different to the 
> "not full" check implemented here?
>
> If "tx done" means "there's space to transmit more messages", then 
> consider this:

The mailbox core appears to use this hook as "there's space to transmit
more messages."  The name does seem really confusing.

Download attachment "signature.asc" of type "application/pgp-signature" (819 bytes)