| 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: <e9a46fa1-e934-169b-6fe3-1035a2823039@st.com>
Date: Thu, 11 Jan 2018 13:51:35 +0000
From: Philippe CORNU <philippe.cornu@...com>
To: Brian Norris <briannorris@...omium.org>,
Archit Taneja <architt@...eaurora.org>,
Andrzej Hajda <a.hajda@...sung.com>,
"Laurent Pinchart" <Laurent.pinchart@...asonboard.com>
CC: David Airlie <airlied@...ux.ie>,
Yannick FERTRE <yannick.fertre@...com>,
Vincent ABRIOU <vincent.abriou@...com>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Sean Paul <seanpaul@...omium.org>,
"Nickey Yang" <nickey.yang@...k-chips.com>,
"hl@...k-chips.com" <hl@...k-chips.com>,
"linux-rockchip@...ts.infradead.org"
<linux-rockchip@...ts.infradead.org>,
"mka@...omium.org" <mka@...omium.org>,
"hoegsberg@...il.com" <hoegsberg@...il.com>,
"zyw@...k-chips.com" <zyw@...k-chips.com>,
"xbl@...k-chips.com" <xbl@...k-chips.com>
Subject: Re: [PATCH v2 1/2] drm/bridge/synopsys: dsi: use common
mipi_dsi_create_packet()
Hi Brian & All *DSI DRM experts*,
1) Re-reading this patch, I realize that the returned value of
dw_mipi_dsi_host_transfer() is not correct: we should return the number
of transfered/received bytes...
so I think there are two solutions: fix this in this serie or add a TODO
for later (both solutions are fine to me :-)
2) Digging more into the drm code, the function
mipi_dsi_device_transfer() in drm_mipi_dsi.c is called in the same file
by the 3 following functions: mipi_dsi_shutdown_peripheral(),
mipi_dsi_turn_on_peripheral() &
mipi_dsi_set_maximum_return_packet_size(). All these 3 functions are
expecting "Return: 0 on success or a negative error code on failure."
that is not in line with the transfer function.
So then, we can change the documentation in this file and have instead
"* Return: The number of bytes transmitted on success or a negative
error code on failure." as for mipi_dsi_generic_write()...
Or we can change the source code of these 3 functions to match with the
documentation "Return: 0 on success...".
note: Hopefully, "users" of these 3 functions test the sign of the
return value (or do not use it).
Does anyone have a preferred solutions?
Many thanks
Philippe :-)
On 01/09/2018 09:32 PM, Brian Norris wrote:
> This takes care of 2 TODOs in this driver, by using the common DSI
> packet-marshalling code instead of our custom short/long write code.
> This both saves us some duplicated code and gets us free support for
> command types that weren't already part of our switch block (e.g.,
> MIPI_DSI_GENERIC_LONG_WRITE).
>
> The code logic stays mostly intact, except that it becomes unnecessary
> to split the short/long write functions, and we have to copy data a bit
> more.
>
> Along the way, I noticed that loop bounds were a little odd:
>
> while (DIV_ROUND_UP(len, pld_data_bytes))
>
> This really was just supposed to be 'len != 0', so I made that more
> clear.
>
> Tested on RK3399 with some pending refactoring patches by Nickey Yang,
> to make the Rockchip DSI driver wrap this common driver.
>
> Signed-off-by: Brian Norris <briannorris@...omium.org>
> Reviewed-by: Philippe Cornu <philippe.cornu@...com>
> Tested-by: Philippe Cornu <philippe.cornu@...com>
> ---
> v2:
> * remove "dcs" naming, since these commands handle generic DSI too, not
> just DCS (thanks Philippe)
> * add Philippe's {Tested,Reviewed}-by
> ---
> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 78 ++++++---------------------
> 1 file changed, 16 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index d9cca4fd66ec..ed91e32ee43a 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -136,10 +136,6 @@
> GEN_SW_0P_TX_LP)
>
> #define DSI_GEN_HDR 0x6c
> -/* TODO These 2 defines will be reworked thanks to mipi_dsi_create_packet() */
> -#define GEN_HDATA(data) (((data) & 0xffff) << 8)
> -#define GEN_HTYPE(type) (((type) & 0xff) << 0)
> -
> #define DSI_GEN_PLD_DATA 0x70
>
> #define DSI_CMD_PKT_STATUS 0x74
> @@ -359,44 +355,15 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val)
> return 0;
> }
>
> -static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi,
> - const struct mipi_dsi_msg *msg)
> -{
> - const u8 *tx_buf = msg->tx_buf;
> - u16 data = 0;
> - u32 val;
> -
> - if (msg->tx_len > 0)
> - data |= tx_buf[0];
> - if (msg->tx_len > 1)
> - data |= tx_buf[1] << 8;
> -
> - if (msg->tx_len > 2) {
> - dev_err(dsi->dev, "too long tx buf length %zu for short write\n",
> - msg->tx_len);
> - return -EINVAL;
> - }
> -
> - val = GEN_HDATA(data) | GEN_HTYPE(msg->type);
> - return dw_mipi_dsi_gen_pkt_hdr_write(dsi, val);
> -}
> -
> -static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,
> - const struct mipi_dsi_msg *msg)
> +static int dw_mipi_dsi_write(struct dw_mipi_dsi *dsi,
> + const struct mipi_dsi_packet *packet)
> {
> - const u8 *tx_buf = msg->tx_buf;
> - int len = msg->tx_len, pld_data_bytes = sizeof(u32), ret;
> - u32 hdr_val = GEN_HDATA(msg->tx_len) | GEN_HTYPE(msg->type);
> + const u8 *tx_buf = packet->payload;
> + int len = packet->payload_length, pld_data_bytes = sizeof(u32), ret;
> u32 remainder;
> u32 val;
>
> - if (msg->tx_len < 3) {
> - dev_err(dsi->dev, "wrong tx buf length %zu for long write\n",
> - msg->tx_len);
> - return -EINVAL;
> - }
> -
> - while (DIV_ROUND_UP(len, pld_data_bytes)) {
> + while (len) {
> if (len < pld_data_bytes) {
> remainder = 0;
> memcpy(&remainder, tx_buf, len);
> @@ -419,40 +386,27 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,
> }
> }
>
> - return dw_mipi_dsi_gen_pkt_hdr_write(dsi, hdr_val);
> + remainder = 0;
> + memcpy(&remainder, packet->header, sizeof(packet->header));
> + return dw_mipi_dsi_gen_pkt_hdr_write(dsi, remainder);
> }
>
> static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
> const struct mipi_dsi_msg *msg)
> {
> struct dw_mipi_dsi *dsi = host_to_dsi(host);
> + struct mipi_dsi_packet packet;
> int ret;
>
> - /*
> - * TODO dw drv improvements
> - * use mipi_dsi_create_packet() instead of all following
> - * functions and code (no switch cases, no
> - * dw_mipi_dsi_dcs_short_write(), only the loop in long_write...)
> - * and use packet.header...
> - */
> - dw_mipi_message_config(dsi, msg);
> -
> - switch (msg->type) {
> - case MIPI_DSI_DCS_SHORT_WRITE:
> - case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
> - case MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE:
> - ret = dw_mipi_dsi_dcs_short_write(dsi, msg);
> - break;
> - case MIPI_DSI_DCS_LONG_WRITE:
> - ret = dw_mipi_dsi_dcs_long_write(dsi, msg);
> - break;
> - default:
> - dev_err(dsi->dev, "unsupported message type 0x%02x\n",
> - msg->type);
> - ret = -EINVAL;
> + ret = mipi_dsi_create_packet(&packet, msg);
> + if (ret) {
> + dev_err(dsi->dev, "failed to create packet: %d\n", ret);
> + return ret;
> }
>
> - return ret;
> + dw_mipi_message_config(dsi, msg);
> +
> + return dw_mipi_dsi_write(dsi, &packet);
> }
>
> static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = {
>
Powered by blists - more mailing lists