lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 4 May 2022 13:41:49 +0300 From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org> To: Douglas Anderson <dianders@...omium.org>, dri-devel@...ts.freedesktop.org Cc: Philip Chen <philipchen@...omium.org>, Sankeerth Billakanti <quic_sbillaka@...cinc.com>, Stephen Boyd <swboyd@...omium.org>, freedreno@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org, Hsin-Yi Wang <hsinyi@...omium.org>, Robert Foss <robert.foss@...aro.org>, Abhinav Kumar <quic_abhinavk@...cinc.com>, Alex Deucher <alexander.deucher@....com>, Daniel Vetter <daniel@...ll.ch>, David Airlie <airlied@...ux.ie>, Lyude Paul <lyude@...hat.com>, Thomas Zimmermann <tzimmermann@...e.de>, linux-kernel@...r.kernel.org Subject: Re: [PATCH v2 1/2] drm/dp: Add callbacks to make using DP AUX bus properly easier On 04/05/2022 01:40, Douglas Anderson wrote: > As talked about in this patch in the kerneldoc of > of_dp_aux_populate_ep_device() and also in the past in commit > a1e3667a9835 ("drm/bridge: ti-sn65dsi86: Promote the AUX channel to > its own sub-dev"), it can be difficult for eDP controller drivers to > know when the panel has finished probing when they're using > of_dp_aux_populate_ep_devices(). > > The ti-sn65dsi86 driver managed to solve this because it was already > broken up into a bunch of sub-drivers. That means we could solve the > problem there by adding a new sub-driver to get the panel. We could > use the traditional -EPROBE_DEFER retry mechansim to handle the case > where the panel hadn't probed yet. > > In parade-ps8640 we didn't really solve this. The code just expects > the panel to be ready right away. While reviewing the code originally > I had managed to convince myself it was fine to just expect the panel > right away, but additional testing has shown that not to be the > case. We could fix parade-ps8640 like we did ti-sn65dsi86 but it's > pretty cumbersome (since we're not already broken into multiple > drivers) and requires a bunch of boilerplate code. > > After discussion [1] it seems like the best solution for most people > is: > - Accept that there's always at most one device that will probe as a > result of the DP AUX bus (it may have sub-devices, but there will be > one device _directly_ probed). > - When that device finishes probing, we can just have a call back. > > This patch implements that idea. We'll now take a callback as an > argument to the populate function. To make this easier to land in > pieces, we'll make wrappers for the old functions. The functions with > the new name (which make it clear that we only have one child) will > take the callback and the functions with the old name will temporarily > wrap. > > This patch also includes in a few tiny fixes that I noticed while > working on the code. Specifically: > - We had forgotten a EXPORT_SYMBOL_GPL on the non "devm" populate > function. This can go to a separate patch, so that the fix can be backported to earlier kernels. Please don't forget the Fixes: tag. > - The kerneldoc of dp_aux_ep_dev_release() had incorrectly included a > "Returns" clause. > > [1] https://lore.kernel.org/r/CAD=FV=Ur3afHhsXe7a3baWEnD=MFKFeKRbhFU+bt3P67G0MVzQ@mail.gmail.com > > Signed-off-by: Douglas Anderson <dianders@...omium.org> > --- > > Changes in v2: > - Change to assume exactly one device. > - Have a probe callback instead of an extra sub device. > > drivers/gpu/drm/display/drm_dp_aux_bus.c | 215 +++++++++++++++-------- > include/drm/display/drm_dp_aux_bus.h | 24 ++- > 2 files changed, 164 insertions(+), 75 deletions(-) > > diff --git a/drivers/gpu/drm/display/drm_dp_aux_bus.c b/drivers/gpu/drm/display/drm_dp_aux_bus.c > index dccf3e2ea323..f3412c1c2a3c 100644 > --- a/drivers/gpu/drm/display/drm_dp_aux_bus.c > +++ b/drivers/gpu/drm/display/drm_dp_aux_bus.c > @@ -3,10 +3,10 @@ > * Copyright 2021 Google Inc. > * > * The DP AUX bus is used for devices that are connected over a DisplayPort > - * AUX bus. The devices on the far side of the bus are referred to as > - * endpoints in this code. > + * AUX bus. The device on the far side of the bus is referred to as an > + * endpoint in this code. > * > - * Commonly there is only one device connected to the DP AUX bus: a panel. > + * There is only one device connected to the DP AUX bus: an eDP panel. > * Though historically panels (even DP panels) have been modeled as simple > * platform devices, putting them under the DP AUX bus allows the panel driver > * to perform transactions on that bus. > @@ -22,6 +22,11 @@ > #include <drm/display/drm_dp_aux_bus.h> > #include <drm/display/drm_dp_helper.h> > > +struct dp_aux_ep_device_with_data { > + struct dp_aux_ep_device aux_ep; > + int (*done_probing)(struct drm_dp_aux *aux); > +}; > + > /** > * dp_aux_ep_match() - The match function for the dp_aux_bus. > * @dev: The device to match. > @@ -48,6 +53,8 @@ static int dp_aux_ep_probe(struct device *dev) > { > struct dp_aux_ep_driver *aux_ep_drv = to_dp_aux_ep_drv(dev->driver); > struct dp_aux_ep_device *aux_ep = to_dp_aux_ep_dev(dev); > + struct dp_aux_ep_device_with_data *aux_ep_with_data = > + container_of(aux_ep, struct dp_aux_ep_device_with_data, aux_ep); > int ret; > > ret = dev_pm_domain_attach(dev, true); > @@ -56,7 +63,31 @@ static int dp_aux_ep_probe(struct device *dev) > > ret = aux_ep_drv->probe(aux_ep); > if (ret) > - dev_pm_domain_detach(dev, true); > + goto err_attached; > + > + if (aux_ep_with_data->done_probing) { > + ret = aux_ep_with_data->done_probing(aux_ep->aux); > + if (ret) { > + /* > + * The done_probing() callback can only defer if it's > + * called directly from of_dp_aux_populate_ep_device() > + * because we had no DP AUX children. Flag an error. > + */ > + if (ret == -EPROBE_DEFER) { > + dev_err(dev, > + "DP AUX done_probing() can't defer w/ aux children\n"); > + ret = -EINVAL; > + } > + goto err_probed; > + } > + } > + > + return 0; > +err_probed: > + if (aux_ep_drv->remove) > + aux_ep_drv->remove(aux_ep); > +err_attached: > + dev_pm_domain_detach(dev, true); > > return ret; > } > @@ -66,7 +97,6 @@ static int dp_aux_ep_probe(struct device *dev) > * @dev: The device to remove. > * > * Calls through to the endpoint driver remove. > - * > */ > static void dp_aux_ep_remove(struct device *dev) > { > @@ -120,12 +150,14 @@ ATTRIBUTE_GROUPS(dp_aux_ep_dev); > /** > * dp_aux_ep_dev_release() - Free memory for the dp_aux_ep device > * @dev: The device to free. > - * > - * Return: 0 if no error or negative error code. > */ > static void dp_aux_ep_dev_release(struct device *dev) > { > - kfree(to_dp_aux_ep_dev(dev)); > + struct dp_aux_ep_device *aux_ep = to_dp_aux_ep_dev(dev); > + struct dp_aux_ep_device_with_data *aux_ep_with_data = > + container_of(aux_ep, struct dp_aux_ep_device_with_data, aux_ep); > + > + kfree(aux_ep_with_data); > } > > static struct device_type dp_aux_device_type_type = { > @@ -139,12 +171,14 @@ static struct device_type dp_aux_device_type_type = { > * @dev: The device to destroy. > * @data: Not used > * > - * This is just used as a callback by of_dp_aux_depopulate_ep_devices() and > + * This is just used as a callback by of_dp_aux_depopulate_ep_device() and > * is called for _all_ of the child devices of the device providing the AUX bus. > * We'll only act on those that are of type "dp_aux_bus_type". > * > - * This function is effectively an inverse of what's in the loop > - * in of_dp_aux_populate_ep_devices(). > + * This function is effectively an inverse of what's in > + * of_dp_aux_populate_ep_devices(). NOTE: since we only populate one child > + * then it's expected that only one device will match all the "if" tests in > + * this function and get to the device_unregister(). > * > * Return: 0 if no error or negative error code. > */ > @@ -167,122 +201,159 @@ static int of_dp_aux_ep_destroy(struct device *dev, void *data) > } > > /** > - * of_dp_aux_depopulate_ep_devices() - Undo of_dp_aux_populate_ep_devices > - * @aux: The AUX channel whose devices we want to depopulate > + * of_dp_aux_depopulate_ep_device() - Undo of_dp_aux_populate_ep_device > + * @aux: The AUX channel whose device we want to depopulate > * > - * This will destroy all devices that were created > - * by of_dp_aux_populate_ep_devices(). > + * This will destroy the device that was created > + * by of_dp_aux_populate_ep_device(). > */ > -void of_dp_aux_depopulate_ep_devices(struct drm_dp_aux *aux) > +void of_dp_aux_depopulate_ep_device(struct drm_dp_aux *aux) > { > device_for_each_child_reverse(aux->dev, NULL, of_dp_aux_ep_destroy); > } > -EXPORT_SYMBOL_GPL(of_dp_aux_depopulate_ep_devices); > +EXPORT_SYMBOL_GPL(of_dp_aux_depopulate_ep_device); Small note about the name. What if we change that to something more future-proof? Something like of_dp_aux_depopulate_bus() (and similarly rename other calls)? > > /** > - * of_dp_aux_populate_ep_devices() - Populate the endpoint devices on the DP AUX > - * @aux: The AUX channel whose devices we want to populate. It is required that > + * of_dp_aux_populate_ep_device() - Populate the endpoint device on the DP AUX > + * @aux: The AUX channel whose device we want to populate. It is required that > * drm_dp_aux_init() has already been called for this AUX channel. > + * @done_probing: Callback functions to call after EP device finishes probing. > + * If we have no EP devices this will be called directly. > + * NULL if you don't need a callback. > * > - * This will populate all the devices under the "aux-bus" node of the device > - * providing the AUX channel (AKA aux->dev). > + * This will populate the device (expected to be an eDP panel) under the > + * "aux-bus" node of the device providing the AUX channel (AKA aux->dev). > * > * When this function finishes, it is _possible_ (but not guaranteed) that > - * our sub-devices will have finished probing. It should be noted that if our > - * sub-devices return -EPROBE_DEFER that we will not return any error codes > - * ourselves but our sub-devices will _not_ have actually probed successfully > - * yet. There may be other cases (maybe added in the future?) where sub-devices > - * won't have been probed yet when this function returns, so it's best not to > - * rely on that. > + * our sub-device will have finished probing. It should be noted that if our > + * sub-device returns -EPROBE_DEFER or is probing asynchronously for some > + * reason that we will not return any error codes ourselves but our > + * sub-device will _not_ have actually probed successfully yet. > + * > + * In many cases it's important for the caller of this function to be notified > + * when our sub device finishes probing. Our sub device is expected to be an > + * eDP panel and the caller is expected to be an eDP controller. The eDP > + * controller needs to be able to get a reference to the panel when it finishes > + * probing. For this reason the caller can pass in a function pointer that > + * will be called when our sub-device finishes probing. > * > * If this function succeeds you should later make sure you call > - * of_dp_aux_depopulate_ep_devices() to undo it, or just use the devm version > + * of_dp_aux_depopulate_ep_device() to undo it, or just use the devm version > * of this function. > * > * Return: 0 if no error or negative error code. > */ > -int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux) > +int of_dp_aux_populate_ep_device(struct drm_dp_aux *aux, > + int (*done_probing)(struct drm_dp_aux *aux)) > { > - struct device_node *bus, *np; > + struct device_node *bus = NULL, *np = NULL; > struct dp_aux_ep_device *aux_ep; > + struct dp_aux_ep_device_with_data *aux_ep_with_data; > int ret; > > /* drm_dp_aux_init() should have been called already; warn if not */ > WARN_ON_ONCE(!aux->ddc.algo); > > - if (!aux->dev->of_node) > - return 0; > + if (aux->dev->of_node) > + bus = of_get_child_by_name(aux->dev->of_node, "aux-bus"); > + if (bus) { > + np = of_get_next_available_child(bus, NULL); > + of_node_put(bus); > + } > > - bus = of_get_child_by_name(aux->dev->of_node, "aux-bus"); > - if (!bus) > + /* > + * If no parent "of_node", no "aux-bus" child node, or no available > + * children then we're done. Call the callback (if needed) and return. > + * > + * NOTE: we intentionally pass the return code from done_probing > + * directly out here. eDP controller drivers may want to support > + * panels from old device trees where the panel was an independent > + * platform device. In that case it's expected that done_probing() > + * might need to return -EPROBE_DEFER to our caller. > + */ > + if (!np) { > + if (done_probing) > + return done_probing(aux); I see your point here (and that it makes code simpler). However I'm a little bit uneasy here. What if code this more explicitly in the drivers? Like the following: if (!dev_has_aux_bus()) { ret = panel_ready(....); } else { ... ret = of_dp_aux_populate_ep_device(dp_aux, panel_ready); ....; } This way you won't have to worry about the EPROBE_DEFER. Or you'd rather forbid it explicitly. Why? Consider the following scenario: dp_driver_probe() /* This creates new devices */ done_probing returns -EPROBE_DEFER /* device registration is unwound */ dp_driver_probe returns -EPROBE_DEFER However as the state of devices was chagned, the dp_driver_probe() can be called again and again, ending up with the the same probe loop that we are trying to solve. > return 0; > + } > > - for_each_available_child_of_node(bus, np) { > - if (of_node_test_and_set_flag(np, OF_POPULATED)) > - continue; > + if (of_node_test_and_set_flag(np, OF_POPULATED)) { > + dev_err(aux->dev, "DP AUX EP device already populated\n"); > + ret = -EINVAL; > + goto err_did_get_np; > + } > > - aux_ep = kzalloc(sizeof(*aux_ep), GFP_KERNEL); > - if (!aux_ep) > - continue; > - aux_ep->aux = aux; > + aux_ep_with_data = kzalloc(sizeof(*aux_ep_with_data), GFP_KERNEL); > + if (!aux_ep_with_data) { > + ret = -ENOMEM; > + goto err_did_set_populated; > + } > > - aux_ep->dev.parent = aux->dev; > - aux_ep->dev.bus = &dp_aux_bus_type; > - aux_ep->dev.type = &dp_aux_device_type_type; > - aux_ep->dev.of_node = of_node_get(np); > - dev_set_name(&aux_ep->dev, "aux-%s", dev_name(aux->dev)); > + aux_ep_with_data->done_probing = done_probing; > > - ret = device_register(&aux_ep->dev); > - if (ret) { > - dev_err(aux->dev, "Failed to create AUX EP for %pOF: %d\n", np, ret); > - of_node_clear_flag(np, OF_POPULATED); > - of_node_put(np); > + aux_ep = &aux_ep_with_data->aux_ep; > + aux_ep->aux = aux; > + aux_ep->dev.parent = aux->dev; > + aux_ep->dev.bus = &dp_aux_bus_type; > + aux_ep->dev.type = &dp_aux_device_type_type; > + aux_ep->dev.of_node = of_node_get(np); > + dev_set_name(&aux_ep->dev, "aux-%s", dev_name(aux->dev)); > > - /* > - * As per docs of device_register(), call this instead > - * of kfree() directly for error cases. > - */ > - put_device(&aux_ep->dev); > + ret = device_register(&aux_ep->dev); > + if (ret) { > + dev_err(aux->dev, "Failed to create AUX EP for %pOF: %d\n", np, ret); > > - /* > - * Following in the footsteps of of_i2c_register_devices(), > - * we won't fail the whole function here--we'll just > - * continue registering any other devices we find. > - */ > - } > - } > + /* > + * As per docs of device_register(), call this instead > + * of kfree() directly for error cases. > + */ > + put_device(&aux_ep->dev); > > - of_node_put(bus); > + goto err_did_set_populated; > + } > > return 0; > + > +err_did_set_populated: > + of_node_clear_flag(np, OF_POPULATED); > + > +err_did_get_np: > + of_node_put(np); > + > + return ret; > } > +EXPORT_SYMBOL_GPL(of_dp_aux_populate_ep_device); > > -static void of_dp_aux_depopulate_ep_devices_void(void *data) > +static void of_dp_aux_depopulate_ep_device_void(void *data) > { > - of_dp_aux_depopulate_ep_devices(data); > + of_dp_aux_depopulate_ep_device(data); > } > > /** > - * devm_of_dp_aux_populate_ep_devices() - devm wrapper for of_dp_aux_populate_ep_devices() > - * @aux: The AUX channel whose devices we want to populate > + * devm_of_dp_aux_populate_ep_device() - devm wrapper for of_dp_aux_populate_ep_device() > + * @aux: The AUX channel whose device we want to populate > + * @done_probing: Callback functions to call after EP device finishes probing. > + * If we have no EP devices this will be called directly. > + * NULL if you don't need a callback. > * > * Handles freeing w/ devm on the device "aux->dev". > * > * Return: 0 if no error or negative error code. > */ > -int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux) > +int devm_of_dp_aux_populate_ep_device(struct drm_dp_aux *aux, > + int (*done_probing)(struct drm_dp_aux *aux)) > { > int ret; > > - ret = of_dp_aux_populate_ep_devices(aux); > + ret = of_dp_aux_populate_ep_device(aux, done_probing); > if (ret) > return ret; > > return devm_add_action_or_reset(aux->dev, > - of_dp_aux_depopulate_ep_devices_void, > + of_dp_aux_depopulate_ep_device_void, > aux); > } > -EXPORT_SYMBOL_GPL(devm_of_dp_aux_populate_ep_devices); > +EXPORT_SYMBOL_GPL(devm_of_dp_aux_populate_ep_device); > > int __dp_aux_dp_driver_register(struct dp_aux_ep_driver *drv, struct module *owner) > { > diff --git a/include/drm/display/drm_dp_aux_bus.h b/include/drm/display/drm_dp_aux_bus.h > index 4f19b20b1dd6..c6d5a66f75f2 100644 > --- a/include/drm/display/drm_dp_aux_bus.h > +++ b/include/drm/display/drm_dp_aux_bus.h > @@ -44,9 +44,27 @@ static inline struct dp_aux_ep_driver *to_dp_aux_ep_drv(struct device_driver *dr > return container_of(drv, struct dp_aux_ep_driver, driver); > } > > -int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux); > -void of_dp_aux_depopulate_ep_devices(struct drm_dp_aux *aux); > -int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux); > +int of_dp_aux_populate_ep_device(struct drm_dp_aux *aux, > + int (*done_probing)(struct drm_dp_aux *aux)); > +void of_dp_aux_depopulate_ep_device(struct drm_dp_aux *aux); > +int devm_of_dp_aux_populate_ep_device(struct drm_dp_aux *aux, > + int (*done_probing)(struct drm_dp_aux *aux)); > + > +/* Deprecated versions of the above functions. To be removed when no callers. */ > +static inline int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux) > +{ > + return of_dp_aux_populate_ep_device(aux, NULL); > +} > + > +static inline int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux) > +{ > + return devm_of_dp_aux_populate_ep_device(aux, NULL); > +} > + > +static inline void of_dp_aux_depopulate_ep_devices(struct drm_dp_aux *aux) > +{ > + of_dp_aux_depopulate_ep_device(aux); > +} > > #define dp_aux_dp_driver_register(aux_ep_drv) \ > __dp_aux_dp_driver_register(aux_ep_drv, THIS_MODULE) -- With best wishes Dmitry
Powered by blists - more mailing lists