| Message ID | 20240419211650.2657096-2-jthies@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: ucsi: Update UCSI alternate mode | expand |
> ucsi_register_altmode checks IS_ERR on returned pointer and treats > NULL as valid. This results in a null deref when > trace_ucsi_register_altmode is called. I find that the change description can be improved further. Is another imperative wording desirable? Can it be nicer to use the term “dereference” for the final commit message? Regards, Markus
On Sat, Apr 20, 2024 at 03:15:16PM +0200, Markus Elfring wrote: > > ucsi_register_altmode checks IS_ERR on returned pointer and treats > > NULL as valid. This results in a null deref when > > trace_ucsi_register_altmode is called. > > I find that the change description can be improved further. > Is another imperative wording desirable? > > Can it be nicer to use the term “dereference” for the final commit message? Hi, This is the semi-friendly patch-bot of Greg Kroah-Hartman. Markus, you seem to have sent a nonsensical or otherwise pointless review comment to a patch submission on a Linux kernel developer mailing list. I strongly suggest that you not do this anymore. Please do not bother developers who are actively working to produce patches and features with comments that, in the end, are a waste of time. Patch submitter, please ignore Markus's suggestion; you do not need to follow it at all. The person/bot/AI that sent it is being ignored by almost all Linux kernel maintainers for having a persistent pattern of behavior of producing distracting and pointless commentary, and inability to adapt to feedback. Please feel free to also ignore emails from them. thanks, greg k-h's patch email bot
Hi Jameson, On Fri, Apr 19, 2024 at 09:16:47PM +0000, Jameson Thies wrote: > From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > ucsi_register_altmode checks IS_ERR on returned pointer and treats > NULL as valid. This results in a null deref when > trace_ucsi_register_altmode is called. > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > --- > drivers/usb/typec/ucsi/ucsi.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > index c4d103db9d0f8..c663dce0659ee 100644 > --- a/drivers/usb/typec/ucsi/ucsi.h > +++ b/drivers/usb/typec/ucsi/ucsi.h > @@ -496,7 +496,7 @@ ucsi_register_displayport(struct ucsi_connector *con, > bool override, int offset, > struct typec_altmode_desc *desc) > { > - return NULL; > + return ERR_PTR(-EOPNOTSUPP); > } Hm. This does not look correct to me. Ignoring trace the old code would have returned success if displayport is not compiled in and all altmodes (except for display port) would be registered. With your code ucsi_register_altmodes will always fail and abort altmode registration if it finds a displayport altmode and CONFIG_TYPEC_DP_ALTMODE is not set. I don't think this is what we want. Maybe it is better to guard the trace call with an if? Am I missing something? Best regards, Christian
Hi Christian, thank you for catching this. You are correct, this would prevent correct altmode registration if CONFIG_TYPEC_DP_ALTMODE is not set. There was a miscommunication on our end when setting up this series. The intention was to check for the EOPNOTSUPP response and fall back to default altmode registration when CONFIG_TYPEC_DP_ALTMODE is not set. Sorry about the confusion, I'll fix this in a v2 series shortly. Thanks, Jameson
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h index c4d103db9d0f8..c663dce0659ee 100644 --- a/drivers/usb/typec/ucsi/ucsi.h +++ b/drivers/usb/typec/ucsi/ucsi.h @@ -496,7 +496,7 @@ ucsi_register_displayport(struct ucsi_connector *con, bool override, int offset, struct typec_altmode_desc *desc) { - return NULL; + return ERR_PTR(-EOPNOTSUPP); } static inline void