| Message ID | c92c38b60b1b55b5d72f7f1c718641e1@codeaurora.org (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v2] Bluetooth: btusb: using big-endian definition for board_id in struct qca_version | expand |
Hi Tim, > As we name nvm file by using big-endian for boardID, so align host with it. > > Signed-off-by: Tim Jiang <tjiang@codeaurora.org> > --- > drivers/bluetooth/btusb.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index 46d892bbde62..08a1c6d8390f 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c > @@ -2883,7 +2883,7 @@ struct qca_version { > __le32 rom_version; > __le32 patch_version; > __le32 ram_version; > - __le16 board_id; > + __be16 board_id; > __le16 flag; > __u8 reserved[4]; > } __packed; > @@ -3072,7 +3072,7 @@ static void btusb_generate_qca_nvm_name(char *fwname, size_t max_size, > u16 flag = le16_to_cpu(ver->flag); > > if (((flag >> 8) & 0xff) == QCA_FLAG_MULTI_NVM) { > - u16 board_id = le16_to_cpu(ver->board_id); > + u16 board_id = be16_to_cpu(ver->board_id); > const char *variant; my original comment still stands. Are you sure you are doing this correctly. The in-memory layout of your NVM is mixed little-endian and big-endian? Really? Or do you want to convert back from host endian to big endian? You commit message text suggest that you have to do this: diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index 46d892bbde62..55a33a5fea56 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -3090,7 +3090,7 @@ static void btusb_generate_qca_nvm_name(char *fwname, size_t max_size, rom_version, variant); } else { snprintf(fwname, max_size, "qca/nvm_usb_%08x%s_%04x.bin", - rom_version, variant, board_id); + rom_version, variant, cpu_to_be16(board_id)); } } else { snprintf(fwname, max_size, "qca/nvm_usb_%08x.bin”, And really, I can not do this anymore. Write lengthy commit messages explaining your change in detail. I am not looking a patches anymore until they have a proper paragraph explaining the change and why it is correct. Also this change had v16 before I merged and even in that version I had to fix issues. Please stop wasting my time. I have no idea why this wasn’t caught earlier. It is a fundamental flaw. I am close to just reverting the previous patch since it seems it clearly needs more testing. Regards Marcel
Hi Marcel: yeah , you are right, but in fact we mixing little-endian and big-endian is defined by qc btsoc(have some reason), so host only should be align with it , I will make another change , need your help to review, thank you. regards. tim On 2021-11-03 22:24, Marcel Holtmann wrote: > Hi Tim, > >> As we name nvm file by using big-endian for boardID, so align host >> with it. >> >> Signed-off-by: Tim Jiang <tjiang@codeaurora.org> >> --- >> drivers/bluetooth/btusb.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c >> index 46d892bbde62..08a1c6d8390f 100644 >> --- a/drivers/bluetooth/btusb.c >> +++ b/drivers/bluetooth/btusb.c >> @@ -2883,7 +2883,7 @@ struct qca_version { >> __le32 rom_version; >> __le32 patch_version; >> __le32 ram_version; >> - __le16 board_id; >> + __be16 board_id; >> __le16 flag; >> __u8 reserved[4]; >> } __packed; >> @@ -3072,7 +3072,7 @@ static void btusb_generate_qca_nvm_name(char >> *fwname, size_t max_size, >> u16 flag = le16_to_cpu(ver->flag); >> >> if (((flag >> 8) & 0xff) == QCA_FLAG_MULTI_NVM) { >> - u16 board_id = le16_to_cpu(ver->board_id); >> + u16 board_id = be16_to_cpu(ver->board_id); >> const char *variant; > > my original comment still stands. Are you sure you are doing this > correctly. The in-memory layout of your NVM is mixed little-endian and > big-endian? Really? Or do you want to convert back from host endian to > big endian? > > You commit message text suggest that you have to do this: > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index 46d892bbde62..55a33a5fea56 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c > @@ -3090,7 +3090,7 @@ static void btusb_generate_qca_nvm_name(char > *fwname, size_t max_size, > rom_version, variant); > } else { > snprintf(fwname, max_size, > "qca/nvm_usb_%08x%s_%04x.bin", > - rom_version, variant, board_id); > + rom_version, variant, > cpu_to_be16(board_id)); > } > } else { > snprintf(fwname, max_size, "qca/nvm_usb_%08x.bin”, > > And really, I can not do this anymore. Write lengthy commit messages > explaining your change in detail. I am not looking a patches anymore > until they have a proper paragraph explaining the change and why it is > correct. > > Also this change had v16 before I merged and even in that version I > had to fix issues. Please stop wasting my time. I have no idea why > this wasn’t caught earlier. It is a fundamental flaw. I am close to > just reverting the previous patch since it seems it clearly needs more > testing. > > Regards > > Marcel
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index 46d892bbde62..08a1c6d8390f 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -2883,7 +2883,7 @@ struct qca_version { __le32 rom_version; __le32 patch_version; __le32 ram_version; - __le16 board_id; + __be16 board_id; __le16 flag; __u8 reserved[4]; } __packed; @@ -3072,7 +3072,7 @@ static void btusb_generate_qca_nvm_name(char *fwname, size_t max_size, u16 flag = le16_to_cpu(ver->flag); if (((flag >> 8) & 0xff) == QCA_FLAG_MULTI_NVM) { - u16 board_id = le16_to_cpu(ver->board_id); + u16 board_id = be16_to_cpu(ver->board_id); const char *variant;
As we name nvm file by using big-endian for boardID, so align host with it. Signed-off-by: Tim Jiang <tjiang@codeaurora.org> --- drivers/bluetooth/btusb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) switch (le32_to_cpu(ver->ram_version)) {