> > Tomas, > > Please, see some comments below... > > On 7/28/20 17:29, Winkler, Tomas wrote: > >>>>> > >>>>> Hi Tomas, > >>>>> > >>>>> On 7/28/20 16:41, Tomas Winkler wrote: > >>>>>> Greg please revert, this commit it changes size of struct > >>>>>> wired_cmd_repeater_auth_stream_req_in, this is not what firmware > >>>>>> is expecting. > >>>>> > >>>>> Could you elaborate on what's the firmware expecting, exactly? > >>>> struct wired_cmd_repeater_auth_stream_req_in { > >>>> - struct hdcp2_streamid_type streams[1]; > >>>> + struct hdcp2_streamid_type streams[]; > >>>> } > >>>> > >>>> But then you have, which you haven't changed to + 1 byte = > >>>> mei_cldev_send(cldev, (u8 *)&verify_mprime_in, > >>>> sizeof(verify_mprime_in)); > >>>> > > I don't think the fix for this is to add 1 byte, if any, it seems to be > sizeof(*data- > >streams), or sizeof(struct hdcp2_streamid_type) what needs to be added. > > But it might be better to code something like this, instead: > > diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c > b/drivers/misc/mei/hdcp/mei_hdcp.c > index e6c3dc595617..7fe63c915548 100644 > --- a/drivers/misc/mei/hdcp/mei_hdcp.c > +++ b/drivers/misc/mei/hdcp/mei_hdcp.c > @@ -572,12 +572,12 @@ static int mei_hdcp_verify_mprime(struct device > *dev, > HDCP_2_2_MPRIME_LEN); > drm_hdcp_cpu_to_be24(verify_mprime_in.seq_num_m, data- > >seq_num_m); > memcpy(verify_mprime_in.streams, data->streams, > - (data->k * sizeof(struct hdcp2_streamid_type))); > + (data->k * sizeof(*data->streams))); > > verify_mprime_in.k = cpu_to_be16(data->k); > > byte = mei_cldev_send(cldev, (u8 *)&verify_mprime_in, > - sizeof(verify_mprime_in)); > + struct_size(&verify_mprime_in, streams, > + data->k)); > if (byte < 0) { > dev_dbg(dev, "mei_cldev_send failed. %zd\n", byte); > return byte; > > struct_size(&verify_mprime_in, streams, data->k) will give us the size, in > bytes, of struct wired_cmd_repeater_auth_stream_req_in plus the size in > bytes for the streams[] flexible-array, which is determined by struct > hdcp2_streamid_type and > data->k. > > What do you think? I think I will talk to the firmware guys tomorrow morning...
> > See more comments below... > > >>> > >>> I see, this is the kind of feedback I need from people that knows > >>> the code better. Thanks! > >>> > >>>> But that's not the major point. Point is that we should be able to > >>>> review and test the code before it is merged. You haven't run it, right? > >>>> There is MAINTAINERS file for a reason. > >>>> > >>> > >>> I'm using this command: > >>> > >>> $ scripts/get_maintainer.pl --nokeywords --nogit --nogit-fallback > >>> > >>> and this is the output for both files: > >>> > >>> $ scripts/get_maintainer.pl --nokeywords --nogit --nogit-fallback > >>> drivers/misc/mei/hdcp/mei_hdcp.c Arnd Bergmann <a...@arndb.de> > >>> (supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman > >>> <gre...@linuxfoundation.org> (supporter:CHAR and MISC DRIVERS) > >>> linux- ker...@vger.kernel.org (open list) $ > >>> scripts/get_maintainer.pl --nokeywords -- nogit --nogit-fallback > >>> drivers/misc/mei/hdcp/mei_hdcp.h Arnd Bergmann <a...@arndb.de> > >>> (supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman > >>> <gre...@linuxfoundation.org> (supporter:CHAR and MISC DRIVERS) > >>> linux- ker...@vger.kernel.org (open list) > >> > >> > >>> For some reason you don't appear on the list above. Do you see > >>> anything wrong with the command I'm running to get the maintainers > >>> and > >> lists? > >> > >> Not sure, it should be caught by drivers/misc/mei/* ? > >> Maybe it is not recursive? Need to check the script, frankly I > >> usually check this manually. > >> > >> INTEL MANAGEMENT ENGINE (mei) > >> M: Tomas Winkler <tomas.wink...@intel.com> > >> L: linux-kernel@vger.kernel.org > >> S: Supported > >> F: Documentation/driver-api/mei/* > >> F: drivers/misc/mei/* > >> F: drivers/watchdog/mei_wdt.c > >> F: include/linux/mei_cl_bus.h > >> F: include/uapi/linux/mei.h > >> F: samples/mei/* > >> > > > > It's not recursive, when I add drivers/misc/mei/hdcp/* . It works > > correctly I will post a patch. > > > > Great. I'm glad we got to the root cause of this issue. :) > > Thanks > -- > Gustavo > > > Thanks > > Tomas > > > > > >> > >>> > >>> Thanks > >>> -- > >>> Gustavo > >>> > >>>>> > >>>>>> I really do not appreciate that the code is bypassing driver > >>>>>> maintaner review, I think this is a minimum we can ask for, this > >>>>>> is not for a first time. > >>>>>> > >>>>>> This reverts commit c56967d674e361ebe716e66992e3c5332b25ac1f. > >>>>>> > >>>>>> Cc: Gustavo A. R. Silva <gustavo...@kernel.org> > >>>>>> Signed-off-by: Tomas Winkler <tomas.wink...@intel.com> > >>>>>> --- > >>>>>> drivers/misc/mei/hdcp/mei_hdcp.c | 2 +- > >>>>>> drivers/misc/mei/hdcp/mei_hdcp.h | 2 +- > >>>>>> 2 files changed, 2 insertions(+), 2 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c > >>>>>> b/drivers/misc/mei/hdcp/mei_hdcp.c > >>>>>> index d1d3e025ca0e..e6c3dc595617 100644 > >>>>>> --- a/drivers/misc/mei/hdcp/mei_hdcp.c > >>>>>> +++ b/drivers/misc/mei/hdcp/mei_hdcp.c > >>>>>> @@ -572,7 +572,7 @@ static int mei_hdcp_verify_mprime(struct > >>>>>> device > >>>>> *dev, > >>>>>> HDCP_2_2_MPRIME_LEN); > >>>>>> drm_hdcp_cpu_to_be24(verify_mprime_in.seq_num_m, data- > >>> seq_num_m); > >>>>>> memcpy(verify_mprime_in.streams, data->streams, > >>>>>> - array_size(data->k, sizeof(*data->streams))); > >>>>>> + (data->k * sizeof(struct hdcp2_streamid_type))); > >>>>>> > >>>>>> verify_mprime_in.k = cpu_to_be16(data->k); > >>>>>> > >>>>>> diff --git a/drivers/misc/mei/hdcp/mei_hdcp.h > >>>>>> b/drivers/misc/mei/hdcp/mei_hdcp.h > >>>>>> index 834757f5e072..18ffc773fa18 100644 > >>>>>> --- a/drivers/misc/mei/hdcp/mei_hdcp.h > >>>>>> +++ b/drivers/misc/mei/hdcp/mei_hdcp.h > >>>>>> @@ -358,7 +358,7 @@ struct > >> wired_cmd_repeater_auth_stream_req_in > >>> { > >>>>>> u8 > >>>>> seq_num_m[HDCP_2_2_SEQ_NUM_LEN]; > >>>>>> u8 > m_prime[HDCP_2_2_MPRIME_LEN]; > >>>>>> __be16 k; > >>>>>> - struct hdcp2_streamid_type streams[]; > >>>>>> + struct hdcp2_streamid_type streams[1]; > >>>>>> } __packed; > >>>>>> > >>>>>> struct wired_cmd_repeater_auth_stream_req_out { > >>>>>>