[PATCH v3 10/24] drm/rockchip: dw-mipi-dsi: don't assume buffer is aligned

John Keeping john at metanate.com
Tue Jan 31 11:56:18 UTC 2017


On Mon, 30 Jan 2017 15:08:11 -0500, Sean Paul wrote:

> On Sun, Jan 29, 2017 at 01:24:30PM +0000, John Keeping wrote:
> > By dereferencing the MIPI command buffer as a u32* we rely on it being
> > correctly aligned on ARM, but this may not be the case.  Copy it into a
> > stack variable that will be correctly aligned.
> > 
> > Signed-off-by: John Keeping <john at metanate.com>
> > Reviewed-by: Chris Zhong <zyw at rock-chips.com>
> > ---
> > v3:
> > - Add Chris' Reviewed-by
> > Unchanged in v2
> > 
> >  drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > index 03fc096fe1bd..ddbc037e7ced 100644
> > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > @@ -607,10 +607,10 @@ static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi,
> >  static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,
> >  				      const struct mipi_dsi_msg *msg)
> >  {
> > -	const u32 *tx_buf = msg->tx_buf;
> > -	int len = msg->tx_len, pld_data_bytes = sizeof(*tx_buf), ret;
> > +	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);
> > -	u32 remainder = 0;
> > +	u32 remainder;
> >  	u32 val;
> >  
> >  	if (msg->tx_len < 3) {
> > @@ -621,12 +621,14 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,
> >  
> >  	while (DIV_ROUND_UP(len, pld_data_bytes)) {
> >  		if (len < pld_data_bytes) {
> > +			remainder = 0;
> >  			memcpy(&remainder, tx_buf, len);
> >  			dsi_write(dsi, DSI_GEN_PLD_DATA, remainder);
> >  			len = 0;
> >  		} else {
> > -			dsi_write(dsi, DSI_GEN_PLD_DATA, *tx_buf);
> > -			tx_buf++;
> > +			memcpy(&remainder, tx_buf, pld_data_bytes);
> > +			dsi_write(dsi, DSI_GEN_PLD_DATA, remainder);
> > +			tx_buf += pld_data_bytes;
> >  			len -= pld_data_bytes;
> >  		}  
> 
> 
> You can clean this up further by removing a couple of the locals, the
> conditional and the division:
> 
> while(len < msg->tx_len) {
>         size_t to_write = MIN(msg->tx_len - len, sizeof(remainder));
> 
>         memcpy(&remainder, msg->tx_buf + len, to_write);
>         dsi_write(dsi, DSI_GEN_PLD_DATA, remainder);
>         len += to_write;
> 
>         ...
> }

That's a nice simplification, but it's more than I was trying to do with
this patch.  I can add a cleanup patch on top to simplify the function,
but I don't have that much time to spend on this at the moment and I'd
rather not block this fix because the original code structure could be
improved.


More information about the dri-devel mailing list