endianness problems in fireworks/bebob_maudio

endianness problems in fireworks/bebob_maudio

[Clemens Ladisch]

Hi,

sparse complains:

sound/firewire/fireworks/fireworks_transaction.c:127:18: warning: restricted __be32 degrades to integer

        t = (struct snd_efw_transaction *)data;
        length = min_t(size_t, t->length * sizeof(t->length), length);

't->length' is still a big-endian value.  This means that the driver
ends up always using 'length'.


sound/firewire/bebob/bebob_maudio.c:100:17: warning: incorrect type in initializer (different base types)
sound/firewire/bebob/bebob_maudio.c:100:17:    expected restricted __be32
sound/firewire/bebob/bebob_maudio.c:100:17:    got int

        __be32 cues[3] = {
                MAUDIO_BOOTLOADER_CUE1,
                MAUDIO_BOOTLOADER_CUE2,
                MAUDIO_BOOTLOADER_CUE3
        };

        rcode = fw_run_transaction(device->card, TCODE_WRITE_BLOCK_REQUEST,
                                   device->node_id, device->generation,
                                   device->max_speed, BEBOB_ADDR_REG_REQ,
                                   cues, sizeof(cues));

The three MAUDIO_BOOTLOADER_CUEx values will end up as a different byte
sequence on big-endian machines.  The simplest way to have these twelve
bytes unchanged on the bus is to have a twelve-byte array in the driver.


Regards,
Clemens

Re: endianness problems in fireworks/bebob_maudio

[Takashi Sakamoto]

Hi Clemens,

On Dec 8 2014 06:24, Clemens Ladisch wrote:
> sound/firewire/fireworks/fireworks_transaction.c:127:18: warning: restricted __be32 degrades to integer
> 
>         t = (struct snd_efw_transaction *)data;
>         length = min_t(size_t, t->length * sizeof(t->length), length);
> 
> 't->length' is still a big-endian value.  This means that the driver
> ends up always using 'length'.

Exactly. I agree they should be fixed.

> sound/firewire/bebob/bebob_maudio.c:100:17: warning: incorrect type in initializer (different base types)
> sound/firewire/bebob/bebob_maudio.c:100:17:    expected restricted __be32
> sound/firewire/bebob/bebob_maudio.c:100:17:    got int
> 
>         __be32 cues[3] = {
>                 MAUDIO_BOOTLOADER_CUE1,
>                 MAUDIO_BOOTLOADER_CUE2,
>                 MAUDIO_BOOTLOADER_CUE3
>         };
> 
>         rcode = fw_run_transaction(device->card, TCODE_WRITE_BLOCK_REQUEST,
>                                    device->node_id, device->generation,
>                                    device->max_speed, BEBOB_ADDR_REG_REQ,
>                                    cues, sizeof(cues));
> 
> The three MAUDIO_BOOTLOADER_CUEx values will end up as a different byte
> sequence on big-endian machines.  The simplest way to have these twelve
> bytes unchanged on the bus is to have a twelve-byte array in the driver.

I'm too lazy for these codes. Yes, they should be fixed as long as we
assume this driver is used in big-endian machine.

For my information, Would you tell me the way to generate these sparse
report? I've been working with sparse for my patchset and never see such
reports, furthermore never receive such reports from kernel build bot.


Thanks

Takashi Sakamoto
o-takashi@sakamocchi.jp

Re: endianness problems in fireworks/bebob_maudio

[Clemens Ladisch]

Takashi Sakamoto wrote:
> For my information, Would you tell me the way to generate these sparse
> report?

make C=1 CF=-D__CHECK_ENDIAN__


Regards,
Clemens

Re: endianness problems in fireworks/bebob_maudio

[Takashi Sakamoto]

On Dec 08 2014 17:09, Clemens Ladisch wrote:
> Takashi Sakamoto wrote:
>> For my information, Would you tell me the way to generate these sparse
>> report?
>
> make C=1 CF=-D__CHECK_ENDIAN__

I always check with 'make C=2' without the 'CF' flag.
Firewire drivers handle big-endian data, so the flag is nice.

Thanks


Takashi Sakamoto
o-takashi@sakamocchi.jp

[PATCH] ALSA: fireworks: fix an endianness bug for transaction length

[Takashi Sakamoto]

Although the 't->length' is a big-endian value, it's used without any
conversion. This means that the driver always uses 'length' parameter.

Fixes: 555e8a8f7f14("ALSA: fireworks: Add command/response functionality into hwdep interface")
Reported-by: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/fireworks/fireworks_transaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/firewire/fireworks/fireworks_transaction.c b/sound/firewire/fireworks/fireworks_transaction.c
index 255dabc..2a85e42 100644
--- a/sound/firewire/fireworks/fireworks_transaction.c
+++ b/sound/firewire/fireworks/fireworks_transaction.c
@@ -124,7 +124,7 @@ copy_resp_to_buf(struct snd_efw *efw, void *data, size_t length, int *rcode)
 	spin_lock_irq(&efw->lock);
 
 	t = (struct snd_efw_transaction *)data;
-	length = min_t(size_t, t->length * sizeof(t->length), length);
+	length = min_t(size_t, be32_to_cpu(t->length) * sizeof(u32), length);
 
 	if (efw->push_ptr < efw->pull_ptr)
 		capacity = (unsigned int)(efw->pull_ptr - efw->push_ptr);
-- 
2.1.0

Re: [PATCH] ALSA: fireworks: fix an endianness bug for transaction length

[Takashi Iwai]

At Thu,  8 Jan 2015 00:31:16 +0900,
Takashi Sakamoto wrote:
> 
> Although the 't->length' is a big-endian value, it's used without any
> conversion. This means that the driver always uses 'length' parameter.
> 
> Fixes: 555e8a8f7f14("ALSA: fireworks: Add command/response functionality into hwdep interface")
> Reported-by: Clemens Ladisch <clemens@ladisch.de>
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Applied, thanks.


Takashi

> ---
>  sound/firewire/fireworks/fireworks_transaction.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/firewire/fireworks/fireworks_transaction.c b/sound/firewire/fireworks/fireworks_transaction.c
> index 255dabc..2a85e42 100644
> --- a/sound/firewire/fireworks/fireworks_transaction.c
> +++ b/sound/firewire/fireworks/fireworks_transaction.c
> @@ -124,7 +124,7 @@ copy_resp_to_buf(struct snd_efw *efw, void *data, size_t length, int *rcode)
>  	spin_lock_irq(&efw->lock);
>  
>  	t = (struct snd_efw_transaction *)data;
> -	length = min_t(size_t, t->length * sizeof(t->length), length);
> +	length = min_t(size_t, be32_to_cpu(t->length) * sizeof(u32), length);
>  
>  	if (efw->push_ptr < efw->pull_ptr)
>  		capacity = (unsigned int)(efw->pull_ptr - efw->push_ptr);
> -- 
> 2.1.0
>