[2/4] soundwire: cdns: fix check for number of messages

Message ID 20190328134134.22479-3-srinivas.kandagatla@linaro.org
State New
Headers show
Series
  • soundwire: few trival fixes and cleanups.
Related show

Commit Message

Srinivas Kandagatla March 28, 2019, 1:41 p.m.
Looks like there is a typo while checking number of messages, which
should be checked in defer pointer rather than message length.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

---
 drivers/soundwire/cadence_master.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.21.0

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Comments

Pierre-Louis Bossart March 28, 2019, 2:03 p.m. | #1
On 3/28/19 9:41 AM, Srinivas Kandagatla wrote:
> Looks like there is a typo while checking number of messages, which

> should be checked in defer pointer rather than message length.

> 

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> ---

>   drivers/soundwire/cadence_master.c | 2 +-

>   1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c

> index cb6a331f448a..d41adbc57918 100644

> --- a/drivers/soundwire/cadence_master.c

> +++ b/drivers/soundwire/cadence_master.c

> @@ -434,7 +434,7 @@ cdns_xfer_msg_defer(struct sdw_bus *bus,

>   	int cmd = 0, ret;

>   

>   	/* for defer only 1 message is supported */

> -	if (msg->len > 1)


I am not sure this is a typo.

IRRC the hardware only defer e.g. a single write that can perform bank 
switches and writes at specific times indicated with an SSP. What's more 
is that the defer structure is actually modified below this code (not 
shown in the diff) to set defer>length = msg->len, so testing before the 
value is set looks more suspicious than the current code.

Vinod, does this ring a bell?


> +	if (defer->length > 1)

>   		return -ENOTSUPP;

>   

>   	ret = cdns_prep_msg(cdns, msg, &cmd);

> 


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Srinivas Kandagatla March 28, 2019, 2:58 p.m. | #2
On 28/03/2019 14:03, Pierre-Louis Bossart wrote:
>> ---
>>   drivers/soundwire/cadence_master.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/soundwire/cadence_master.c 
>> b/drivers/soundwire/cadence_master.c
>> index cb6a331f448a..d41adbc57918 100644
>> --- a/drivers/soundwire/cadence_master.c
>> +++ b/drivers/soundwire/cadence_master.c
>> @@ -434,7 +434,7 @@ cdns_xfer_msg_defer(struct sdw_bus *bus,
>>       int cmd = 0, ret;
>>       /* for defer only 1 message is supported */
>> -    if (msg->len > 1)
> 
> I am not sure this is a typo.
> 

Am bit confused with defer data-structure itself.

I was assuming that defer->length is number of sdw messages in the 
structure. If its same as message lenght then sdw_msg->len and 
sdw_defer->length are totally redundant. May be we should get rid of 
lenght field in defer to avoid confusion?

> IRRC the hardware only defer e.g. a single write that can perform bank 
> switches and writes at specific times indicated with an SSP. What's more 
okay got it.
Does that mean that candence controller can only support 1 byte defer 
transfers?

> is that the defer structure is actually modified below this code (not 
> shown in the diff) to set defer>length = msg->len, so testing before the 
> value is set looks more suspicious than the current code.

removing the length field in defer should get rid of such instances.

--srini

> 
> Vinod, does this ring a bell?
> 
> 
>> +    if (defer->length > 1)
>>           return -ENOTSUPP;
>>       ret = cdns_prep_msg(cdns, msg, &cmd);
Sanyog Kale March 29, 2019, 3:51 a.m. | #3
On Thu, Mar 28, 2019 at 02:58:27PM +0000, Srinivas Kandagatla wrote:
> 

> 

> On 28/03/2019 14:03, Pierre-Louis Bossart wrote:

> > > ---

> > > ?? drivers/soundwire/cadence_master.c | 2 +-

> > > ?? 1 file changed, 1 insertion(+), 1 deletion(-)

> > > 

> > > diff --git a/drivers/soundwire/cadence_master.c

> > > b/drivers/soundwire/cadence_master.c

> > > index cb6a331f448a..d41adbc57918 100644

> > > --- a/drivers/soundwire/cadence_master.c

> > > +++ b/drivers/soundwire/cadence_master.c

> > > @@ -434,7 +434,7 @@ cdns_xfer_msg_defer(struct sdw_bus *bus,

> > > ?????????? int cmd = 0, ret;

> > > ?????????? /* for defer only 1 message is supported */

> > > -?????? if (msg->len > 1)

> > 

> > I am not sure this is a typo.

> > 

> 

> Am bit confused with defer data-structure itself.

> 

> I was assuming that defer->length is number of sdw messages in the

> structure. If its same as message lenght then sdw_msg->len and

> sdw_defer->length are totally redundant. May be we should get rid of lenght

> field in defer to avoid confusion?

> 

> > IRRC the hardware only defer e.g. a single write that can perform bank

> > switches and writes at specific times indicated with an SSP. What's more

> okay got it.

> Does that mean that candence controller can only support 1 byte defer

> transfers?

> 

> > is that the defer structure is actually modified below this code (not

> > shown in the diff) to set defer>length = msg->len, so testing before the

> > value is set looks more suspicious than the current code.

> 

> removing the length field in defer should get rid of such instances.

>


defer->length is passed as count in cdns_fill_msg_resp function while
processing response of defer message. we can as well use msg->len and
remove length from sdw_defer. I dont see any other instance where
defer->length is used.

> --srini

> 

> > 

> > Vinod, does this ring a bell?

> > 

> > 

> > > +?????? if (defer->length > 1)

> > > ?????????????????? return -ENOTSUPP;

> > > ?????????? ret = cdns_prep_msg(cdns, msg, &cmd);


-- 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Vinod Koul March 29, 2019, 5:54 a.m. | #4
On 28-03-19, 14:58, Srinivas Kandagatla wrote:
> 

> 

> On 28/03/2019 14:03, Pierre-Louis Bossart wrote:

> > > ---

> > >   drivers/soundwire/cadence_master.c | 2 +-

> > >   1 file changed, 1 insertion(+), 1 deletion(-)

> > > 

> > > diff --git a/drivers/soundwire/cadence_master.c

> > > b/drivers/soundwire/cadence_master.c

> > > index cb6a331f448a..d41adbc57918 100644

> > > --- a/drivers/soundwire/cadence_master.c

> > > +++ b/drivers/soundwire/cadence_master.c

> > > @@ -434,7 +434,7 @@ cdns_xfer_msg_defer(struct sdw_bus *bus,

> > >       int cmd = 0, ret;

> > >       /* for defer only 1 message is supported */

> > > -    if (msg->len > 1)

> > 

> > I am not sure this is a typo.

> > 

> 

> Am bit confused with defer data-structure itself.

> 

> I was assuming that defer->length is number of sdw messages in the

> structure. If its same as message lenght then sdw_msg->len and

> sdw_defer->length are totally redundant. May be we should get rid of lenght

> field in defer to avoid confusion?


Sorry Srini, I didnt pay much attention to it last time we discussed. So
relooking at the code, we send msg as a message to trf (defered or not)
and then copy length few lines down, but I agree this is not most useful
and should be cleaned up.

> > IRRC the hardware only defer e.g. a single write that can perform bank

> > switches and writes at specific times indicated with an SSP. What's more

> okay got it.

> Does that mean that candence controller can only support 1 byte defer

> transfers?


I think so, Pierre can confirm.

> > is that the defer structure is actually modified below this code (not

> > shown in the diff) to set defer>length = msg->len, so testing before the

> > value is set looks more suspicious than the current code.

> 

> removing the length field in defer should get rid of such instances.


Right!

-- 
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Srinivas Kandagatla March 29, 2019, 10:06 a.m. | #5
On 29/03/2019 05:54, Vinod Koul wrote:
>>> is that the defer structure is actually modified below this code (not

>>> shown in the diff) to set defer>length = msg->len, so testing before the

>>> value is set looks more suspicious than the current code.

>> removing the length field in defer should get rid of such instances.

> Right!

I will go ahead and post the cleanup patch to get rid of 
confusing/redundant length field in defer.

Thanks,
srini
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Patch

diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
index cb6a331f448a..d41adbc57918 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -434,7 +434,7 @@  cdns_xfer_msg_defer(struct sdw_bus *bus,
 	int cmd = 0, ret;
 
 	/* for defer only 1 message is supported */
-	if (msg->len > 1)
+	if (defer->length > 1)
 		return -ENOTSUPP;
 
 	ret = cdns_prep_msg(cdns, msg, &cmd);