| Message ID | 0010a6165f3560f16123a142d297276e7d6c2087.1620952511.git.connojdavis@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/4] usb: early: Avoid using DbC if already enabled | expand |
On 5/13/21 8:56 PM, Connor Davis wrote: > Callers of dbgp_reset_prep treat a 0 return value as "stop using > the debug port", which means they don't make any subsequent calls to > dbgp_reset_prep or dbgp_external_startup. > > To ensure the callers' interpretation is correct, first return -EPERM > from xen_dbgp_op if !xen_initial_domain(). This ensures that > both xen_dbgp_reset_prep and xen_dbgp_external_startup return 0 > iff the PHYSDEVOP_DBGP_RESET_{PREPARE,DONE} hypercalls succeed. Also > update xen_dbgp_reset_prep and xen_dbgp_external_startup to return > -EPERM when !CONFIG_XEN_DOM0 for consistency. > > Next, return nonzero from dbgp_reset_prep if xen_dbgp_reset_prep returns > 0. The nonzero value ensures that callers of dbgp_reset_prep will > subsequently call dbgp_external_startup when it is safe to do so. > > Also invert the return values from dbgp_external_startup for > consistency with dbgp_reset_prep (this inversion has no functional > change since no callers actually check the value). > > Signed-off-by: Connor Davis <connojdavis@gmail.com> For Xen bits: Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> For the rest it seems to me that error code passing could be improved: if it's 0 or 1 it should be bool. Or pass actual error code, with zero for no-error case, such as ... > --- > drivers/usb/early/ehci-dbgp.c | 9 ++++++--- > drivers/xen/dbgp.c | 2 +- > include/linux/usb/ehci-dbgp.h | 14 +++++++++----- > 3 files changed, 16 insertions(+), 9 deletions(-) > > diff --git a/drivers/usb/early/ehci-dbgp.c b/drivers/usb/early/ehci-dbgp.c > index 45b42d8f6453..ff993d330c01 100644 > --- a/drivers/usb/early/ehci-dbgp.c > +++ b/drivers/usb/early/ehci-dbgp.c > @@ -970,8 +970,8 @@ int dbgp_reset_prep(struct usb_hcd *hcd) > int ret = xen_dbgp_reset_prep(hcd); > u32 ctrl; > > - if (ret) > - return ret; > + if (!ret) > + return 1; ... here or ... > > dbgp_not_safe = 1; > if (!ehci_debug) > @@ -995,7 +995,10 @@ EXPORT_SYMBOL_GPL(dbgp_reset_prep); > > int dbgp_external_startup(struct usb_hcd *hcd) > { > - return xen_dbgp_external_startup(hcd) ?: _dbgp_external_startup(); > + if (!xen_dbgp_external_startup(hcd)) > + return 1; ... here. -boris
diff --git a/drivers/usb/early/ehci-dbgp.c b/drivers/usb/early/ehci-dbgp.c index 45b42d8f6453..ff993d330c01 100644 --- a/drivers/usb/early/ehci-dbgp.c +++ b/drivers/usb/early/ehci-dbgp.c @@ -970,8 +970,8 @@ int dbgp_reset_prep(struct usb_hcd *hcd) int ret = xen_dbgp_reset_prep(hcd); u32 ctrl; - if (ret) - return ret; + if (!ret) + return 1; dbgp_not_safe = 1; if (!ehci_debug) @@ -995,7 +995,10 @@ EXPORT_SYMBOL_GPL(dbgp_reset_prep); int dbgp_external_startup(struct usb_hcd *hcd) { - return xen_dbgp_external_startup(hcd) ?: _dbgp_external_startup(); + if (!xen_dbgp_external_startup(hcd)) + return 1; + + return !_dbgp_external_startup(); } EXPORT_SYMBOL_GPL(dbgp_external_startup); #endif /* USB */ diff --git a/drivers/xen/dbgp.c b/drivers/xen/dbgp.c index fef32dd1a5dc..d54f98380e6f 100644 --- a/drivers/xen/dbgp.c +++ b/drivers/xen/dbgp.c @@ -15,7 +15,7 @@ static int xen_dbgp_op(struct usb_hcd *hcd, int op) struct physdev_dbgp_op dbgp; if (!xen_initial_domain()) - return 0; + return -EPERM; dbgp.op = op; diff --git a/include/linux/usb/ehci-dbgp.h b/include/linux/usb/ehci-dbgp.h index 62ab3805172d..c0e98557efc0 100644 --- a/include/linux/usb/ehci-dbgp.h +++ b/include/linux/usb/ehci-dbgp.h @@ -56,28 +56,32 @@ extern int xen_dbgp_external_startup(struct usb_hcd *); #else static inline int xen_dbgp_reset_prep(struct usb_hcd *hcd) { - return 1; /* Shouldn't this be 0? */ + return -EPERM; } static inline int xen_dbgp_external_startup(struct usb_hcd *hcd) { - return -1; + return -EPERM; } #endif #ifdef CONFIG_EARLY_PRINTK_DBGP -/* Call backs from ehci host driver to ehci debug driver */ +/* + * Call backs from ehci host driver to ehci debug driver. + * Returns 0 if the debug port should stopped being used, + * nonzero otherwise. + */ extern int dbgp_external_startup(struct usb_hcd *); extern int dbgp_reset_prep(struct usb_hcd *); #else static inline int dbgp_reset_prep(struct usb_hcd *hcd) { - return xen_dbgp_reset_prep(hcd); + return !xen_dbgp_reset_prep(hcd); } static inline int dbgp_external_startup(struct usb_hcd *hcd) { - return xen_dbgp_external_startup(hcd); + return !xen_dbgp_external_startup(hcd); } #endif
Callers of dbgp_reset_prep treat a 0 return value as "stop using the debug port", which means they don't make any subsequent calls to dbgp_reset_prep or dbgp_external_startup. To ensure the callers' interpretation is correct, first return -EPERM from xen_dbgp_op if !xen_initial_domain(). This ensures that both xen_dbgp_reset_prep and xen_dbgp_external_startup return 0 iff the PHYSDEVOP_DBGP_RESET_{PREPARE,DONE} hypercalls succeed. Also update xen_dbgp_reset_prep and xen_dbgp_external_startup to return -EPERM when !CONFIG_XEN_DOM0 for consistency. Next, return nonzero from dbgp_reset_prep if xen_dbgp_reset_prep returns 0. The nonzero value ensures that callers of dbgp_reset_prep will subsequently call dbgp_external_startup when it is safe to do so. Also invert the return values from dbgp_external_startup for consistency with dbgp_reset_prep (this inversion has no functional change since no callers actually check the value). Signed-off-by: Connor Davis <connojdavis@gmail.com> --- drivers/usb/early/ehci-dbgp.c | 9 ++++++--- drivers/xen/dbgp.c | 2 +- include/linux/usb/ehci-dbgp.h | 14 +++++++++----- 3 files changed, 16 insertions(+), 9 deletions(-)