[PATCH 1/2] mailbox: Avoid NULL-pointer dereference in mbox_chan_received_data()

[PATCH 1/2] mailbox: Avoid NULL-pointer dereference in mbox_chan_received_data()

[Andrew Bresticker]

If a message has been received on a channel, but no client has yet bound
to it, mbox_chan_received_data() will dereference a NULL client pointer.
Check for the presence of a client first.

Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
---
 drivers/mailbox/mailbox.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index afcb430..5008028 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -142,7 +142,7 @@ static void poll_txdone(unsigned long data)
 void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
 {
 	/* No buffering the received data */
-	if (chan->cl->rx_callback)
+	if (chan->cl && chan->cl->rx_callback)
 		chan->cl->rx_callback(chan->cl, mssg);
 }
 EXPORT_SYMBOL_GPL(mbox_chan_received_data);
-- 
2.1.0.rc2.206.gedb03e5

[PATCH 1/2] mailbox: Avoid NULL-pointer dereference in mbox_chan_received_data()

[Andrew Bresticker]

If a message has been received on a channel, but no client has yet bound
to it, mbox_chan_received_data() will dereference a NULL client pointer.
Check for the presence of a client first.

Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
---
 drivers/mailbox/mailbox.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index afcb430..5008028 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -142,7 +142,7 @@ static void poll_txdone(unsigned long data)
 void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
 {
 	/* No buffering the received data */
-	if (chan->cl->rx_callback)
+	if (chan->cl && chan->cl->rx_callback)
 		chan->cl->rx_callback(chan->cl, mssg);
 }
 EXPORT_SYMBOL_GPL(mbox_chan_received_data);
-- 
2.1.0.rc2.206.gedb03e5

[PATCH 2/2] mailbox: Don't unnecessarily re-arm the polling timer

[Andrew Bresticker]

poll_txdone() will unconditionally re-arm the polling timer if there was
an active request, even if the active request completed and no other
requests were submitted.  This is fixed by:
 - only re-arming the timer if the controller reported that the current
   transmission has not completed, and,
 - moving the call to poll_txdone() into msg_submit() so that the
   controller gets polled (and the timer re-armed, if necessary) whenever
   a new message is submitted.

Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
---
 drivers/mailbox/mailbox.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 5008028..26f74ad 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -28,6 +28,8 @@
 static LIST_HEAD(mbox_cons);
 static DEFINE_MUTEX(con_mutex);
 
+static void poll_txdone(unsigned long data);
+
 static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
 {
 	int idx;
@@ -60,7 +62,7 @@ static void msg_submit(struct mbox_chan *chan)
 	unsigned count, idx;
 	unsigned long flags;
 	void *data;
-	int err;
+	int err = -EBUSY;
 
 	spin_lock_irqsave(&chan->lock, flags);
 
@@ -84,6 +86,9 @@ static void msg_submit(struct mbox_chan *chan)
 	}
 exit:
 	spin_unlock_irqrestore(&chan->lock, flags);
+
+	if (!err && chan->txdone_method == TXDONE_BY_POLL)
+		poll_txdone((unsigned long)chan->mbox);
 }
 
 static void tx_tick(struct mbox_chan *chan, int r)
@@ -117,10 +122,11 @@ static void poll_txdone(unsigned long data)
 		struct mbox_chan *chan = &mbox->chans[i];
 
 		if (chan->active_req && chan->cl) {
-			resched = true;
 			txdone = chan->mbox->ops->last_tx_done(chan);
 			if (txdone)
 				tx_tick(chan, 0);
+			else
+				resched = true;
 		}
 	}
 
@@ -252,9 +258,6 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
 
 	msg_submit(chan);
 
-	if (chan->txdone_method	== TXDONE_BY_POLL)
-		poll_txdone((unsigned long)chan->mbox);
-
 	if (chan->cl->tx_block && chan->active_req) {
 		unsigned long wait;
 		int ret;
-- 
2.1.0.rc2.206.gedb03e5

[PATCH 2/2] mailbox: Don't unnecessarily re-arm the polling timer

[Andrew Bresticker]

poll_txdone() will unconditionally re-arm the polling timer if there was
an active request, even if the active request completed and no other
requests were submitted.  This is fixed by:
 - only re-arming the timer if the controller reported that the current
   transmission has not completed, and,
 - moving the call to poll_txdone() into msg_submit() so that the
   controller gets polled (and the timer re-armed, if necessary) whenever
   a new message is submitted.

Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
---
 drivers/mailbox/mailbox.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 5008028..26f74ad 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -28,6 +28,8 @@
 static LIST_HEAD(mbox_cons);
 static DEFINE_MUTEX(con_mutex);
 
+static void poll_txdone(unsigned long data);
+
 static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
 {
 	int idx;
@@ -60,7 +62,7 @@ static void msg_submit(struct mbox_chan *chan)
 	unsigned count, idx;
 	unsigned long flags;
 	void *data;
-	int err;
+	int err = -EBUSY;
 
 	spin_lock_irqsave(&chan->lock, flags);
 
@@ -84,6 +86,9 @@ static void msg_submit(struct mbox_chan *chan)
 	}
 exit:
 	spin_unlock_irqrestore(&chan->lock, flags);
+
+	if (!err && chan->txdone_method == TXDONE_BY_POLL)
+		poll_txdone((unsigned long)chan->mbox);
 }
 
 static void tx_tick(struct mbox_chan *chan, int r)
@@ -117,10 +122,11 @@ static void poll_txdone(unsigned long data)
 		struct mbox_chan *chan = &mbox->chans[i];
 
 		if (chan->active_req && chan->cl) {
-			resched = true;
 			txdone = chan->mbox->ops->last_tx_done(chan);
 			if (txdone)
 				tx_tick(chan, 0);
+			else
+				resched = true;
 		}
 	}
 
@@ -252,9 +258,6 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
 
 	msg_submit(chan);
 
-	if (chan->txdone_method	== TXDONE_BY_POLL)
-		poll_txdone((unsigned long)chan->mbox);
-
 	if (chan->cl->tx_block && chan->active_req) {
 		unsigned long wait;
 		int ret;
-- 
2.1.0.rc2.206.gedb03e5

[PATCH 2/2] mailbox: Don't unnecessarily re-arm the polling timer

[Thierry Reding]

On Thu, Oct 30, 2014 at 01:01:07PM -0700, Andrew Bresticker wrote:
> poll_txdone() will unconditionally re-arm the polling timer if there was
> an active request, even if the active request completed and no other
> requests were submitted.  This is fixed by:
>  - only re-arming the timer if the controller reported that the current
>    transmission has not completed, and,
>  - moving the call to poll_txdone() into msg_submit() so that the
>    controller gets polled (and the timer re-armed, if necessary) whenever
>    a new message is submitted.
> 
> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
> ---
>  drivers/mailbox/mailbox.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 5008028..26f74ad 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -28,6 +28,8 @@
>  static LIST_HEAD(mbox_cons);
>  static DEFINE_MUTEX(con_mutex);
>  
> +static void poll_txdone(unsigned long data);

I think I'd rather move poll_txdone() here to avoid the forward
declaration, but either way:

Reviewed-by: Thierry Reding <treding@nvidia.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141031/5002600c/attachment-0001.sig>

Re: [PATCH 2/2] mailbox: Don't unnecessarily re-arm the polling timer

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1203 bytes --]

On Thu, Oct 30, 2014 at 01:01:07PM -0700, Andrew Bresticker wrote:
> poll_txdone() will unconditionally re-arm the polling timer if there was
> an active request, even if the active request completed and no other
> requests were submitted.  This is fixed by:
>  - only re-arming the timer if the controller reported that the current
>    transmission has not completed, and,
>  - moving the call to poll_txdone() into msg_submit() so that the
>    controller gets polled (and the timer re-armed, if necessary) whenever
>    a new message is submitted.
> 
> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
> ---
>  drivers/mailbox/mailbox.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 5008028..26f74ad 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -28,6 +28,8 @@
>  static LIST_HEAD(mbox_cons);
>  static DEFINE_MUTEX(con_mutex);
>  
> +static void poll_txdone(unsigned long data);

I think I'd rather move poll_txdone() here to avoid the forward
declaration, but either way:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/2] mailbox: Don't unnecessarily re-arm the polling timer

[Jassi Brar]

On 31 October 2014 17:11, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Thu, Oct 30, 2014 at 01:01:07PM -0700, Andrew Bresticker wrote:
>> poll_txdone() will unconditionally re-arm the polling timer if there was
>> an active request, even if the active request completed and no other
>> requests were submitted.  This is fixed by:
>>  - only re-arming the timer if the controller reported that the current
>>    transmission has not completed, and,
>>  - moving the call to poll_txdone() into msg_submit() so that the
>>    controller gets polled (and the timer re-armed, if necessary) whenever
>>    a new message is submitted.
>>
>> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
>> ---
>>  drivers/mailbox/mailbox.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>> index 5008028..26f74ad 100644
>> --- a/drivers/mailbox/mailbox.c
>> +++ b/drivers/mailbox/mailbox.c
>> @@ -28,6 +28,8 @@
>>  static LIST_HEAD(mbox_cons);
>>  static DEFINE_MUTEX(con_mutex);
>>
>> +static void poll_txdone(unsigned long data);
>
> I think I'd rather move poll_txdone() here to avoid the forward
> declaration, but either way:
>
> Reviewed-by: Thierry Reding <treding@nvidia.com>
>
Does the 'extra' timer fire cause some issue? I believe it shouldn't.
Anyways, I have applied the patch.

Thanks
Jassi

Re: [PATCH 2/2] mailbox: Don't unnecessarily re-arm the polling timer

[Jassi Brar]

On 31 October 2014 17:11, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Thu, Oct 30, 2014 at 01:01:07PM -0700, Andrew Bresticker wrote:
>> poll_txdone() will unconditionally re-arm the polling timer if there was
>> an active request, even if the active request completed and no other
>> requests were submitted.  This is fixed by:
>>  - only re-arming the timer if the controller reported that the current
>>    transmission has not completed, and,
>>  - moving the call to poll_txdone() into msg_submit() so that the
>>    controller gets polled (and the timer re-armed, if necessary) whenever
>>    a new message is submitted.
>>
>> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
>> ---
>>  drivers/mailbox/mailbox.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>> index 5008028..26f74ad 100644
>> --- a/drivers/mailbox/mailbox.c
>> +++ b/drivers/mailbox/mailbox.c
>> @@ -28,6 +28,8 @@
>>  static LIST_HEAD(mbox_cons);
>>  static DEFINE_MUTEX(con_mutex);
>>
>> +static void poll_txdone(unsigned long data);
>
> I think I'd rather move poll_txdone() here to avoid the forward
> declaration, but either way:
>
> Reviewed-by: Thierry Reding <treding@nvidia.com>
>
Does the 'extra' timer fire cause some issue? I believe it shouldn't.
Anyways, I have applied the patch.

Thanks
Jassi

[PATCH 1/2] mailbox: Avoid NULL-pointer dereference in mbox_chan_received_data()

[Jassi Brar]

On 31 October 2014 01:31, Andrew Bresticker <abrestic@chromium.org> wrote:
> If a message has been received on a channel, but no client has yet bound
> to it, mbox_chan_received_data() will dereference a NULL client pointer.
> Check for the presence of a client first.
>
Let me quote from the documentation of the API ....
/**
   ....
 * After startup and before shutdown any data received on the chan
 * is passed on to the API via atomic mbox_chan_received_data().
 * The controller should ACK the RX only after this call returns.
 */
Please note "after startup and before shutdown".

We can sure suppress the crash by returning from
mbox_chan_received_data() but would that be neat? Because the real
problem lies with the controller driver that pushes data even from a
mailbox that nobody has 'enabled'.  I can see your virtual-channel
implementation needs to maintain a field for each such channel, but
for physically discreet channels it would usually be a simple matter
of setting/clearing a bit (IRQ Enable/Disable).

However, I think even for your case, you could simply set/clear the
'con_priv' instead of 'vchan_allocated' and use that hint whether to
push RX data up to the core or not.

Thanks
Jassi

Re: [PATCH 1/2] mailbox: Avoid NULL-pointer dereference in mbox_chan_received_data()

[Jassi Brar]

On 31 October 2014 01:31, Andrew Bresticker <abrestic@chromium.org> wrote:
> If a message has been received on a channel, but no client has yet bound
> to it, mbox_chan_received_data() will dereference a NULL client pointer.
> Check for the presence of a client first.
>
Let me quote from the documentation of the API ....
/**
   ....
 * After startup and before shutdown any data received on the chan
 * is passed on to the API via atomic mbox_chan_received_data().
 * The controller should ACK the RX only after this call returns.
 */
Please note "after startup and before shutdown".

We can sure suppress the crash by returning from
mbox_chan_received_data() but would that be neat? Because the real
problem lies with the controller driver that pushes data even from a
mailbox that nobody has 'enabled'.  I can see your virtual-channel
implementation needs to maintain a field for each such channel, but
for physically discreet channels it would usually be a simple matter
of setting/clearing a bit (IRQ Enable/Disable).

However, I think even for your case, you could simply set/clear the
'con_priv' instead of 'vchan_allocated' and use that hint whether to
push RX data up to the core or not.

Thanks
Jassi