| Message ID | 20240416-ucsi-glink-altmode-v1-0-890db00877ac@linaro.org |
|---|---|
| Headers | show |
| Series | usb: typec: ucsi: glink: merge in altmode support | expand |
On 4/16/24 04:20, Dmitry Baryshkov wrote: > The driver gets data from the DSP firmware. Sanitize data size before > reading corresponding message structures. > > Fixes: 62b5412b1f4a ("usb: typec: ucsi: add PMIC Glink UCSI driver") > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- I think more backstory would be beneficial here.. Does this happen often? What are the consequences? What are the causes? Can there be one-off invalid messages, or does that mean the firwmare has entered some unstable state? And I suppose, if answer to the last question is "unstable state", are we doing something incorrectly in Linux that causes it to happen? Konrad
On 4/16/24 04:20, Dmitry Baryshkov wrote: > All platforms except Qualcomm SC8180X pass CCI in the notification > message. Use it instead of going back and forth over RPMSG > interface to read CCI. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- Are we sure it's reeeeallly just 8180? Konrad
On Tue, 16 Apr 2024 at 17:33, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > On 4/16/24 04:20, Dmitry Baryshkov wrote: > > The driver gets data from the DSP firmware. Sanitize data size before > > reading corresponding message structures. > > > > Fixes: 62b5412b1f4a ("usb: typec: ucsi: add PMIC Glink UCSI driver") > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > I think more backstory would be beneficial here.. Does this happen often? > What are the consequences? What are the causes? Can there be one-off invalid > messages, or does that mean the firwmare has entered some unstable state? > > And I suppose, if answer to the last question is "unstable state", are we > doing something incorrectly in Linux that causes it to happen? No. I haven't seen such cases. However as we are getting the data from external entity which don't control, we should check that the messages conform to what we are expecting.
On Tue, 16 Apr 2024 at 17:36, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > On 4/16/24 04:20, Dmitry Baryshkov wrote: > > All platforms except Qualcomm SC8180X pass CCI in the notification > > message. Use it instead of going back and forth over RPMSG > > interface to read CCI. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > Are we sure it's reeeeallly just 8180? More or less so. Previous platforms used either qcom-pmic-typec (sm8150, sm8250) or laptop-specific EC (sdm850 / c630, sc7180 / aspire1, CrOS). Next platforms (sc8280xp and sm8350, qcm6490) used pmic-glink and the new way of sending events. I think sc8180x was really the unfortunate one to use mixed events.
On Tue, Apr 16, 2024 at 05:20:50AM +0300, Dmitry Baryshkov wrote: > Make typec_set_mode() also handle retimers in addition to muxes. Setting > the USB mode requires retimers to be configured in addition to just > switching the mux configuration. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/typec/class.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c > index 9610e647a8d4..28d395535bd1 100644 > --- a/drivers/usb/typec/class.c > +++ b/drivers/usb/typec/class.c > @@ -2095,14 +2095,21 @@ EXPORT_SYMBOL_GPL(typec_get_orientation); > * @mode: Accessory Mode, USB Operation or Safe State > * > * Configure @port for Accessory Mode @mode. This function will configure the > - * muxes needed for @mode. > + * muxes and retimeres needed for @mode. > */ > int typec_set_mode(struct typec_port *port, int mode) > { > + struct typec_retimer_state retimer_state = { }; > struct typec_mux_state state = { }; > + int ret; > > + retimer_state.mode = mode; > state.mode = mode; > > + ret = typec_retimer_set(port->retimer, &retimer_state); > + if (ret) > + return ret; > + > return typec_mux_set(port->mux, &state); > } > EXPORT_SYMBOL_GPL(typec_set_mode); > > -- > 2.39.2
PMIC GLINK platforms handle the altmode support via OOB messages rather than by fully following the UCSI standard. Currently altmode handling is implemented in a separate driver, which has to duplicate significant part of the USB-C stack to control USB-C muxes, switches and retimers. Also this potentially introduces race conditions, since both UCSI and pmic-glink-altmode will drive those components. Last but not least, there is no connection betnween the altmode's aux-hpd-bridge and corresponding typec_port instance. Merge the pmic-glink-altmode driver into the ucsi-glink, streamling the altmode support for Qualcomm platforms. Depends: https://lore.kernel.org/linux-usb/20240411-ucsi-orient-aware-v2-0-d4b1cb22a33f@linaro.org/ Merge strategy: since the series involves both UCSI and soc/qcom drivers, I'd kindly ask to ack merging the whole patchset through the USB tree. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- Dmitry Baryshkov (8): usb: typec: Handle retimers in typec_set_mode() usb: typec: altmode: add low level altmode configuration helper usb: typec: ucsi: glink: check message data sizes usb: typec: ucsi: glink: use le32 for message data usb: typec: ucsi: glink: simplify notification handling usb: typec: ucsi: add ucsi_registered() callback usb: typec: ucsi: glink: merge pmic_glink_altmode driver soc: qcom: pmic-glink: drop separate altmode driver support drivers/soc/qcom/Makefile | 1 - drivers/soc/qcom/pmic_glink.c | 15 +- drivers/soc/qcom/pmic_glink_altmode.c | 546 ------------------------------ drivers/usb/typec/bus.c | 34 ++ drivers/usb/typec/class.c | 9 +- drivers/usb/typec/ucsi/ucsi.c | 3 + drivers/usb/typec/ucsi/ucsi.h | 2 + drivers/usb/typec/ucsi/ucsi_glink.c | 615 +++++++++++++++++++++++++++++++--- include/linux/usb/typec_altmode.h | 3 + 9 files changed, 619 insertions(+), 609 deletions(-) --- base-commit: 7f3fd687151a552038967f31993f1bc7e447b99e change-id: 20240408-ucsi-glink-altmode-293cdbf3270c Best regards,