| 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
| ||
|
Message-ID: <ZtWjEudmlR51zkU9@smile.fi.intel.com>
Date: Mon, 2 Sep 2024 14:35:46 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Stephen Boyd <swboyd@...omium.org>
Cc: chrome-platform@...ts.linux.dev, linux-kernel@...r.kernel.org,
patches@...ts.linux.dev, devicetree@...r.kernel.org,
Douglas Anderson <dianders@...omium.org>,
Pin-yen Lin <treapking@...omium.org>,
Andrzej Hajda <andrzej.hajda@...el.com>,
Benson Leung <bleung@...omium.org>,
Conor Dooley <conor+dt@...nel.org>, Daniel Vetter <daniel@...ll.ch>,
David Airlie <airlied@...il.com>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
dri-devel@...ts.freedesktop.org,
Guenter Roeck <groeck@...omium.org>,
Jernej Skrabec <jernej.skrabec@...il.com>,
Jonas Karlman <jonas@...boo.se>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Lee Jones <lee@...nel.org>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Neil Armstrong <neil.armstrong@...aro.org>,
Prashant Malani <pmalani@...omium.org>,
Robert Foss <rfoss@...nel.org>, Rob Herring <robh+dt@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
Tzung-Bi Shih <tzungbi@...nel.org>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Daniel Scally <djrscally@...il.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Ivan Orlov <ivan.orlov0322@...il.com>, linux-acpi@...r.kernel.org,
linux-usb@...r.kernel.org,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
"Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Vinod Koul <vkoul@...nel.org>
Subject: Re: [PATCH v4 06/18] drm/bridge: aux-hpd: Support USB Type-C DP
altmodes via DRM lane assignment
On Sat, Aug 31, 2024 at 09:06:44PM -0700, Stephen Boyd wrote:
> Extend the aux-hpd bridge driver to support assigning DP lanes to USB
> type-c pins based on typec mux state entry. Existing users of this
> driver only need the HPD signaling support, so leave that in place and
> wrap the code with a variant that supports more features of USB type-c
Isn't the proper spelling "USB Type-C"?
> DP altmode, i.e. pin configurations. Prefix that code with
> 'drm_dp_typec_bridge' to differentiate it from the existing
> 'drm_aux_hpd_bridge' code.
>
> Parse the struct typec_mux_state members to determine if DP altmode has
> been entered and if HPD is asserted or not. Signal HPD to the drm bridge
> chain when HPD is asserted. Similarly, parse the pin assignment and map
> the DP lanes to the usb-c output lanes, taking into account any lane
> remapping from the data-lanes endpoint property. Pass that lane mapping
> to the previous drm_bridge in the bridge chain during the atomic check
> phase.
...
> +static inline struct drm_dp_typec_bridge_data *
> +hpd_bridge_to_typec_bridge_data(struct drm_aux_hpd_bridge_data *hpd_data)
> +{
> + return container_of(hpd_data, struct drm_dp_typec_bridge_data, hpd_bridge);
+ container_of.h ?
With that said, can the argument be const here?
> +}
...
Ditto for the two more helpers, added in this change.
...
> +static void drm_dp_typec_bridge_release(struct device *dev)
> +{
> + struct drm_dp_typec_bridge_dev *typec_bridge_dev;
> + struct auxiliary_device *adev;
> +
> + typec_bridge_dev = to_drm_dp_typec_bridge_dev(dev);
> + adev = &typec_bridge_dev->adev;
> +
> + ida_free(&drm_aux_hpd_bridge_ida, adev->id);
> + of_node_put(adev->dev.platform_data);
> + of_node_put(adev->dev.of_node);
I'm wondering why it's not made fwnode to begin with...
>From the file / function names it's not obvious that it's OF-only code. Neither
there is no explanations why this must be OF-only code (among all fwnode types
supported).
> + kfree(typec_bridge_dev);
> +}
...
> + return ERR_PTR(dev_err_probe(parent, -ENODEV, "Missing typec endpoint(s) port@0\n"));
We have a new helper for such cases.
...
> + adev->dev.of_node = of_node_get(parent->of_node);
device_set_node() ?
...
> + ret = auxiliary_device_init(adev);
> + if (ret) {
> + of_node_put(adev->dev.platform_data);
> + of_node_put(adev->dev.of_node);
> + ida_free(&drm_aux_hpd_bridge_ida, adev->id);
> + kfree(adev);
> + return ERR_PTR(ret);
Can cleanup.h be utilised here and in other error paths in this function?
> + }
> + ret = devm_add_action_or_reset(parent, drm_aux_hpd_bridge_free_adev, adev);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + return typec_bridge_dev;
> +}
...
> +static int dp_lane_to_typec_lane(enum dp_lane lane)
> +{
> + switch (lane) {
> + case DP_ML0:
> + return USB_SSTX2;
> + case DP_ML1:
> + return USB_SSRX2;
> + case DP_ML2:
> + return USB_SSTX1;
> + case DP_ML3:
> + return USB_SSRX1;
> + }
> + return -EINVAL;
Hmm... This can be simply made as default case.
> +}
> +
> +static int typec_to_dp_lane(enum usb_ss_lane lane)
> +{
> + switch (lane) {
> + case USB_SSRX1:
> + return DP_ML3;
> + case USB_SSTX1:
> + return DP_ML2;
> + case USB_SSTX2:
> + return DP_ML0;
> + case USB_SSRX2:
> + return DP_ML1;
> + }
> +
> + return -EINVAL;
Ditto.
> +}
...
> + for (i = 0; i < num_lanes; i++) {
> + /* Get physical type-c lane for DP lane */
> + typec_lane = dp_lane_to_typec_lane(i);
> + if (typec_lane < 0) {
> + dev_err(&adev->dev, "Invalid type-c lane configuration at DP_ML%d\n", i);
> + return -EINVAL;
Most likely typec_lane contains an error code here. If yes, it would be rather
return typec_lane;
If no, why not make that happen?
> + }
> +
> + /* Map physical to logical type-c lane */
> + typec_lane = lane_mapping[typec_lane];
> +
> + /* Map logical type-c lane to logical DP lane */
> + dp_lanes[i] = typec_to_dp_lane(typec_lane);
> + }
...
> +static int drm_dp_typec_bridge_atomic_check(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state)
> +{
> + struct drm_dp_typec_bridge_data *data;
> + struct drm_lane_cfg *in_lanes;
> + u8 *dp_lanes;
> + size_t num_lanes;
> + int i;
Does it need to be signed? Can it theoretically overflow as num_lanes defined
as size_t?
> + data = to_drm_dp_typec_bridge_data(bridge);
> + num_lanes = data->num_lanes;
> + if (!num_lanes)
> + return 0;
> + dp_lanes = data->dp_lanes;
> +
> + in_lanes = kcalloc(num_lanes, sizeof(*in_lanes), GFP_KERNEL);
> + if (!in_lanes)
> + return -ENOMEM;
> +
> + bridge_state->input_bus_cfg.lanes = in_lanes;
> + bridge_state->input_bus_cfg.num_lanes = num_lanes;
> +
> + for (i = 0; i < num_lanes; i++)
> + in_lanes[i].logical = dp_lanes[i];
> +
> + return 0;
> +}
...
> + port->typec_data = typec_data;
> + if (of_property_read_u32_array(ep.local_node, "data-lanes",
> + port->lane_mapping,
> + ARRAY_SIZE(port->lane_mapping))) {
> + memcpy(port->lane_mapping, mapping, sizeof(mapping));
Hmm... I'm wondering if direct assignment will save a few .text bytes
port->lane_mapping = ...;
of_property_read_u32_array(ep.local_node, "data-lanes",
port->lane_mapping,
ARRAY_SIZE(port->lane_mapping));
Also note that conditional is not needed in this case.
(And again, why OF-centric code?)
> + }
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists