* [PATCH] avrcp: Remove dead code
@ 2014-01-10 14:56 Andrei Emeltchenko
2014-01-13 9:08 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 3+ messages in thread
From: Andrei Emeltchenko @ 2014-01-10 14:56 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
session->target cannot be NULL since it is already checked 11 lines
above:
...
if (session == NULL || session->target == NULL)
return -ENOTCONN;
...
---
profiles/audio/avrcp.c | 24 ++++--------------------
1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 8d4309a..197959f 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -3722,30 +3722,14 @@ int avrcp_set_volume(struct btd_device *dev, uint8_t volume)
DBG("volume=%u", volume);
- if (session->target) {
- pdu->pdu_id = AVRCP_SET_ABSOLUTE_VOLUME;
- pdu->params[0] = volume;
- pdu->params_len = htons(1);
+ pdu->pdu_id = AVRCP_SET_ABSOLUTE_VOLUME;
+ pdu->params[0] = volume;
+ pdu->params_len = htons(1);
- return avctp_send_vendordep_req(session->conn,
+ return avctp_send_vendordep_req(session->conn,
AVC_CTYPE_CONTROL, AVC_SUBUNIT_PANEL,
buf, sizeof(buf),
avrcp_handle_set_volume, session);
- } else if (session->registered_events &
- (1 << AVRCP_EVENT_VOLUME_CHANGED)) {
- uint8_t id = AVRCP_EVENT_VOLUME_CHANGED;
- pdu->pdu_id = AVRCP_REGISTER_NOTIFICATION;
- pdu->params[0] = AVRCP_EVENT_VOLUME_CHANGED;
- pdu->params[1] = volume;
- pdu->params_len = htons(2);
-
- return avctp_send_vendordep(session->conn,
- session->transaction_events[id],
- AVC_CTYPE_CHANGED, AVC_SUBUNIT_PANEL,
- buf, sizeof(buf));
- }
-
- return 0;
}
static int avrcp_connect(struct btd_service *service)
--
1.8.3.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] avrcp: Remove dead code
2014-01-10 14:56 [PATCH] avrcp: Remove dead code Andrei Emeltchenko
@ 2014-01-13 9:08 ` Luiz Augusto von Dentz
2014-01-13 10:55 ` Andrei Emeltchenko
0 siblings, 1 reply; 3+ messages in thread
From: Luiz Augusto von Dentz @ 2014-01-13 9:08 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth@vger.kernel.org
Hi Andrei,
On Fri, Jan 10, 2014 at 4:56 PM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> session->target cannot be NULL since it is already checked 11 lines
> above:
> ...
> if (session == NULL || session->target == NULL)
> return -ENOTCONN;
> ...
> ---
> profiles/audio/avrcp.c | 24 ++++--------------------
> 1 file changed, 4 insertions(+), 20 deletions(-)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index 8d4309a..197959f 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -3722,30 +3722,14 @@ int avrcp_set_volume(struct btd_device *dev, uint8_t volume)
>
> DBG("volume=%u", volume);
>
> - if (session->target) {
> - pdu->pdu_id = AVRCP_SET_ABSOLUTE_VOLUME;
> - pdu->params[0] = volume;
> - pdu->params_len = htons(1);
> + pdu->pdu_id = AVRCP_SET_ABSOLUTE_VOLUME;
> + pdu->params[0] = volume;
> + pdu->params_len = htons(1);
>
> - return avctp_send_vendordep_req(session->conn,
> + return avctp_send_vendordep_req(session->conn,
> AVC_CTYPE_CONTROL, AVC_SUBUNIT_PANEL,
> buf, sizeof(buf),
> avrcp_handle_set_volume, session);
> - } else if (session->registered_events &
> - (1 << AVRCP_EVENT_VOLUME_CHANGED)) {
> - uint8_t id = AVRCP_EVENT_VOLUME_CHANGED;
> - pdu->pdu_id = AVRCP_REGISTER_NOTIFICATION;
> - pdu->params[0] = AVRCP_EVENT_VOLUME_CHANGED;
> - pdu->params[1] = volume;
> - pdu->params_len = htons(2);
> -
> - return avctp_send_vendordep(session->conn,
> - session->transaction_events[id],
> - AVC_CTYPE_CHANGED, AVC_SUBUNIT_PANEL,
> - buf, sizeof(buf));
> - }
The above code is necessary to notify volume changes when acting as
controller (session->control is set), so your fix is not good either
and we should depend on session->target on the first place because
both session->control and session->target can be set.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] avrcp: Remove dead code
2014-01-13 9:08 ` Luiz Augusto von Dentz
@ 2014-01-13 10:55 ` Andrei Emeltchenko
0 siblings, 0 replies; 3+ messages in thread
From: Andrei Emeltchenko @ 2014-01-13 10:55 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org
Hi Luiz,
On Mon, Jan 13, 2014 at 11:08:49AM +0200, Luiz Augusto von Dentz wrote:
> Hi Andrei,
>
> On Fri, Jan 10, 2014 at 4:56 PM, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> > session->target cannot be NULL since it is already checked 11 lines
> > above:
> > ...
> > if (session == NULL || session->target == NULL)
> > return -ENOTCONN;
> > ...
> > ---
> > profiles/audio/avrcp.c | 24 ++++--------------------
> > 1 file changed, 4 insertions(+), 20 deletions(-)
> >
> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > index 8d4309a..197959f 100644
> > --- a/profiles/audio/avrcp.c
> > +++ b/profiles/audio/avrcp.c
> > @@ -3722,30 +3722,14 @@ int avrcp_set_volume(struct btd_device *dev, uint8_t volume)
> >
> > DBG("volume=%u", volume);
> >
> > - if (session->target) {
> > - pdu->pdu_id = AVRCP_SET_ABSOLUTE_VOLUME;
> > - pdu->params[0] = volume;
> > - pdu->params_len = htons(1);
> > + pdu->pdu_id = AVRCP_SET_ABSOLUTE_VOLUME;
> > + pdu->params[0] = volume;
> > + pdu->params_len = htons(1);
> >
> > - return avctp_send_vendordep_req(session->conn,
> > + return avctp_send_vendordep_req(session->conn,
> > AVC_CTYPE_CONTROL, AVC_SUBUNIT_PANEL,
> > buf, sizeof(buf),
> > avrcp_handle_set_volume, session);
> > - } else if (session->registered_events &
> > - (1 << AVRCP_EVENT_VOLUME_CHANGED)) {
> > - uint8_t id = AVRCP_EVENT_VOLUME_CHANGED;
> > - pdu->pdu_id = AVRCP_REGISTER_NOTIFICATION;
> > - pdu->params[0] = AVRCP_EVENT_VOLUME_CHANGED;
> > - pdu->params[1] = volume;
> > - pdu->params_len = htons(2);
> > -
> > - return avctp_send_vendordep(session->conn,
> > - session->transaction_events[id],
> > - AVC_CTYPE_CHANGED, AVC_SUBUNIT_PANEL,
> > - buf, sizeof(buf));
> > - }
>
> The above code is necessary to notify volume changes when acting as
> controller (session->control is set), so your fix is not good either
> and we should depend on session->target on the first place because
> both session->control and session->target can be set.
So did you make a proper fix, since otherwise this is dead code?
Best regards
Andrei Emeltchenko
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-01-13 10:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-10 14:56 [PATCH] avrcp: Remove dead code Andrei Emeltchenko
2014-01-13 9:08 ` Luiz Augusto von Dentz
2014-01-13 10:55 ` Andrei Emeltchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox