* [bug report] usb: typec: ucsi: Only enable supported notifications
@ 2024-05-08 7:53 Dan Carpenter
2024-05-08 14:22 ` Diogo Ivo
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2024-05-08 7:53 UTC (permalink / raw
To: diogo.ivo; +Cc: linux-usb
Hello Diogo Ivo,
Commit 27ffe4ff0b33 ("usb: typec: ucsi: Only enable supported
notifications") from Mar 27, 2024 (linux-next), leads to the
following Smatch static checker warning:
drivers/usb/typec/ucsi/ucsi.c:1671 ucsi_get_supported_notifications()
warn: was expecting a 64 bit value instead of '((((1))) << (24))'
drivers/usb/typec/ucsi/ucsi.c
1665 static u64 ucsi_get_supported_notifications(struct ucsi *ucsi)
1666 {
1667 u8 features = ucsi->cap.features;
1668 u64 ntfy = UCSI_ENABLE_NTFY_ALL;
1669
1670 if (!(features & UCSI_CAP_ALT_MODE_DETAILS))
--> 1671 ntfy &= ~UCSI_ENABLE_NTFY_CAM_CHANGE;
ntfy is a u64 but UCSI_ENABLE_NTFY_CAM_CHANGE is unsigned long. So on
a 32 bit system this will clear the high 32 bits. So far as I can see
ntfy should just be a u32. Either way, the types should match.
BIT_ULL() is the way to do that if it really needs to be a u64.
1672
1673 if (!(features & UCSI_CAP_PDO_DETAILS))
1674 ntfy &= ~(UCSI_ENABLE_NTFY_PWR_LEVEL_CHANGE |
1675 UCSI_ENABLE_NTFY_CAP_CHANGE);
1676
1677 if (!(features & UCSI_CAP_EXT_SUPPLY_NOTIFICATIONS))
1678 ntfy &= ~UCSI_ENABLE_NTFY_EXT_PWR_SRC_CHANGE;
1679
1680 if (!(features & UCSI_CAP_PD_RESET))
1681 ntfy &= ~UCSI_ENABLE_NTFY_PD_RESET_COMPLETE;
1682
1683 return ntfy;
1684 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] usb: typec: ucsi: Only enable supported notifications 2024-05-08 7:53 [bug report] usb: typec: ucsi: Only enable supported notifications Dan Carpenter @ 2024-05-08 14:22 ` Diogo Ivo 2024-05-08 14:34 ` Dan Carpenter 0 siblings, 1 reply; 4+ messages in thread From: Diogo Ivo @ 2024-05-08 14:22 UTC (permalink / raw To: Dan Carpenter; +Cc: linux-usb, heikki.krogerus, diogo.ivo Hello Dan, On Wed, May 08, 2024 at 10:53:05AM GMT, Dan Carpenter wrote: > Hello Diogo Ivo, > > Commit 27ffe4ff0b33 ("usb: typec: ucsi: Only enable supported > notifications") from Mar 27, 2024 (linux-next), leads to the > following Smatch static checker warning: > > drivers/usb/typec/ucsi/ucsi.c:1671 ucsi_get_supported_notifications() > warn: was expecting a 64 bit value instead of '((((1))) << (24))' > > drivers/usb/typec/ucsi/ucsi.c > 1665 static u64 ucsi_get_supported_notifications(struct ucsi *ucsi) > 1666 { > 1667 u8 features = ucsi->cap.features; > 1668 u64 ntfy = UCSI_ENABLE_NTFY_ALL; > 1669 > 1670 if (!(features & UCSI_CAP_ALT_MODE_DETAILS)) > --> 1671 ntfy &= ~UCSI_ENABLE_NTFY_CAM_CHANGE; > > ntfy is a u64 but UCSI_ENABLE_NTFY_CAM_CHANGE is unsigned long. So on > a 32 bit system this will clear the high 32 bits. So far as I can see > ntfy should just be a u32. Either way, the types should match. > BIT_ULL() is the way to do that if it really needs to be a u64. In my view this variable really should be a u64 and the definitions of the UCSI_ENABLE_NTFY_* need to be changed to BIT_ULL(). This is due to UCSI versions 2.0 and up definining a new notification on bit 33, crossing the u32 barrier. My suggestion for addressing this is sending two patches, one for changing BIT() -> BIT_ULL() and adding the missing define for the notification of bit 33 and a separate patch to handle this new notification bit in ucsi_get_supported_notifications()/ucsi_init(). Thank you for the report and please let me know if this sounds reasonable, or if it would be better to split the changes in another way. Best regards, Diogo ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] usb: typec: ucsi: Only enable supported notifications 2024-05-08 14:22 ` Diogo Ivo @ 2024-05-08 14:34 ` Dan Carpenter 2024-05-08 14:45 ` Diogo Ivo 0 siblings, 1 reply; 4+ messages in thread From: Dan Carpenter @ 2024-05-08 14:34 UTC (permalink / raw To: Diogo Ivo; +Cc: linux-usb, heikki.krogerus On Wed, May 08, 2024 at 03:22:49PM +0100, Diogo Ivo wrote: > Hello Dan, > > On Wed, May 08, 2024 at 10:53:05AM GMT, Dan Carpenter wrote: > > Hello Diogo Ivo, > > > > Commit 27ffe4ff0b33 ("usb: typec: ucsi: Only enable supported > > notifications") from Mar 27, 2024 (linux-next), leads to the > > following Smatch static checker warning: > > > > drivers/usb/typec/ucsi/ucsi.c:1671 ucsi_get_supported_notifications() > > warn: was expecting a 64 bit value instead of '((((1))) << (24))' > > > > drivers/usb/typec/ucsi/ucsi.c > > 1665 static u64 ucsi_get_supported_notifications(struct ucsi *ucsi) > > 1666 { > > 1667 u8 features = ucsi->cap.features; > > 1668 u64 ntfy = UCSI_ENABLE_NTFY_ALL; > > 1669 > > 1670 if (!(features & UCSI_CAP_ALT_MODE_DETAILS)) > > --> 1671 ntfy &= ~UCSI_ENABLE_NTFY_CAM_CHANGE; > > > > ntfy is a u64 but UCSI_ENABLE_NTFY_CAM_CHANGE is unsigned long. So on > > a 32 bit system this will clear the high 32 bits. So far as I can see > > ntfy should just be a u32. Either way, the types should match. > > BIT_ULL() is the way to do that if it really needs to be a u64. > > In my view this variable really should be a u64 and the definitions of > the UCSI_ENABLE_NTFY_* need to be changed to BIT_ULL(). This is due to > UCSI versions 2.0 and up definining a new notification on bit 33, crossing > the u32 barrier. My suggestion for addressing this is sending two > patches, one for changing BIT() -> BIT_ULL() and adding the missing > define for the notification of bit 33 and a separate patch to handle > this new notification bit in ucsi_get_supported_notifications()/ucsi_init(). > > Thank you for the report and please let me know if this sounds > reasonable, or if it would be better to split the changes in another > way. Yes, this sounds reasonable to me. I don't know the hardware at all and didn't know you were planning to add a BIT(33). regards, dan carpenter ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] usb: typec: ucsi: Only enable supported notifications 2024-05-08 14:34 ` Dan Carpenter @ 2024-05-08 14:45 ` Diogo Ivo 0 siblings, 0 replies; 4+ messages in thread From: Diogo Ivo @ 2024-05-08 14:45 UTC (permalink / raw To: Dan Carpenter; +Cc: linux-usb, heikki.krogerus, diogo.ivo On Wed, May 08, 2024 at 05:34:57PM GMT, Dan Carpenter wrote: > On Wed, May 08, 2024 at 03:22:49PM +0100, Diogo Ivo wrote: > > Hello Dan, > > > > On Wed, May 08, 2024 at 10:53:05AM GMT, Dan Carpenter wrote: > > > Hello Diogo Ivo, > > > > > > Commit 27ffe4ff0b33 ("usb: typec: ucsi: Only enable supported > > > notifications") from Mar 27, 2024 (linux-next), leads to the > > > following Smatch static checker warning: > > > > > > drivers/usb/typec/ucsi/ucsi.c:1671 ucsi_get_supported_notifications() > > > warn: was expecting a 64 bit value instead of '((((1))) << (24))' > > > > > > drivers/usb/typec/ucsi/ucsi.c > > > 1665 static u64 ucsi_get_supported_notifications(struct ucsi *ucsi) > > > 1666 { > > > 1667 u8 features = ucsi->cap.features; > > > 1668 u64 ntfy = UCSI_ENABLE_NTFY_ALL; > > > 1669 > > > 1670 if (!(features & UCSI_CAP_ALT_MODE_DETAILS)) > > > --> 1671 ntfy &= ~UCSI_ENABLE_NTFY_CAM_CHANGE; > > > > > > ntfy is a u64 but UCSI_ENABLE_NTFY_CAM_CHANGE is unsigned long. So on > > > a 32 bit system this will clear the high 32 bits. So far as I can see > > > ntfy should just be a u32. Either way, the types should match. > > > BIT_ULL() is the way to do that if it really needs to be a u64. > > > > In my view this variable really should be a u64 and the definitions of > > the UCSI_ENABLE_NTFY_* need to be changed to BIT_ULL(). This is due to > > UCSI versions 2.0 and up definining a new notification on bit 33, crossing > > the u32 barrier. My suggestion for addressing this is sending two > > patches, one for changing BIT() -> BIT_ULL() and adding the missing > > define for the notification of bit 33 and a separate patch to handle > > this new notification bit in ucsi_get_supported_notifications()/ucsi_init(). > > > > Thank you for the report and please let me know if this sounds > > reasonable, or if it would be better to split the changes in another > > way. > > Yes, this sounds reasonable to me. I don't know the hardware at all and > didn't know you were planning to add a BIT(33). To be honest this was not something I had thought about and I looked into it after seeing your report; if this extra notification was not there the best solution would probably be to use u32. Best regards, Diogo ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-05-08 14:45 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-05-08 7:53 [bug report] usb: typec: ucsi: Only enable supported notifications Dan Carpenter 2024-05-08 14:22 ` Diogo Ivo 2024-05-08 14:34 ` Dan Carpenter 2024-05-08 14:45 ` Diogo Ivo
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.