Re: [PATCH v3] drm/dp_mst: Fix W=1 warnings

From: Daniel Vetter <daniel@ffwll.ch>
To: Benjamin GAIGNARD <benjamin.gaignard@st.com>
Cc: Dave Airlie <airlied@gmail.com>, Lyude Paul <lyude@redhat.com>,
	David Airlie <airlied@linux.ie>, Sean Paul <sean@poorly.run>,
	ML dri-devel <dri-devel@lists.freedesktop.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3] drm/dp_mst: Fix W=1 warnings
Date: Mon, 3 Feb 2020 10:40:35 +0100	[thread overview]
Message-ID: <20200203094035.GR43062@phenom.ffwll.local> (raw)
In-Reply-To: <f64197e6-74bd-6577-2aa7-9c69cfdb9080@st.com>

On Fri, Jan 31, 2020 at 08:08:34AM +0000, Benjamin GAIGNARD wrote:
> 
> On 1/31/20 12:22 AM, Dave Airlie wrote:
> >>>> hi-actually yes, we should probably be using this instead of just dropping
> >>>> this. Also, I didn't write this code originally I just refactored a bunch
> >>>> of
> >>>> it - Dave Airlied is the original author, but the original version of this
> >>>> code was written ages ago. tbh, I think it's a safe bet to say that they
> >>>> probably did mean to use this but forgot to and no one noticed until now.
> >>> Hi,
> >>>
> >>> Any clue about how to use crc value ? Does it have to be checked
> >>> against something else ?
> >>> If crc are not matching what should we do of the data copied just before ?
> >> We should be able to just take the CRC value from the sideband message and
> >> then generate our own CRC value using the sideband message contents, and check
> >> if the two are equal. If they aren't, something went wrong and we didn't
> >> receive the message properly.
> >>
> >> Now as to what we should do when we have CRC mismatches? That's a bit more
> >> difficult. If you have access to the DP MST spec, I suppose a place to start
> >> figuring that out would be checking if there's a way for us to request that a
> >> branch device resend whatever message it sent previously. If there isn't, I
> >> guess we should just print an error in dmesg (possibly with a hexdump of the
> >> failed message as well) and not forward the message to the driver. Not sure of
> >> any better way of handling it then that
> > Yeah I think this reflects what I wanted to do, I've no memory of a
> > retransmit option in the spec, but I've away from it for a while. But
> > we'd want to compare the CRC with what we got to make sure the are the
> > same.
> 
> Hmm, that far more complex than just fix compilation warnings :)
> 
> I will split the patch in two:
> 
> - one for of all other warnings, hopefully it can get reviewed
> 
> - one for this crc4 variable. Does checking crc value and print an error 
> should be acceptable ?
> 
> Something like:
> 
> if (crc4 != msg->chunk[msg->curchunk_len - 1])
> 
>      print_hex_dump(KERN_DEBUG, "wrong crc", DUMP_PREFIX_NONE, 16, 1, 
> msg->chunk,  msg->curchunk_len, false);

Yeah I think that should be reasonable as a start. Then we'll see how much
the bug reports start flowing in ...
-Daniel
> 
> 
> Benjamin
> 
> 
> >
> > Dave.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch