* [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.