Re: [char-misc-next] Revert "mei: hdcp: Replace one-element array with flexible-array member"
On 7/28/20 18:16, Gustavo A. R. Silva wrote:
>
>
> On 7/28/20 18:04, Gustavo A. R. Silva wrote:
>> 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.
>>>
Instead of reverting that commit, I think this patch should be applied
to fix it, if Tomas consider this is the appropiate solution:
diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
index d1d3e025ca0e..852374565cba 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,
- array_size(data->k, sizeof(*data->streams)));
+ flex_array_size(_mprime_in, streams, data->k));
verify_mprime_in.k = cpu_to_be16(data->k);
byte = mei_cldev_send(cldev, (u8 *)_mprime_in,
- sizeof(verify_mprime_in));
+ struct_size(_mprime_in, streams, data->k));
if (byte < 0) {
dev_dbg(dev, "mei_cldev_send failed. %zd\n", byte);
return byte;
I'll write up a proper patch after getting some feedback.
Thanks
--
Gustavo
>>> 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 *)_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 *)_mprime_in,
>> - sizeof(verify_mprime_in));
>> + struct_size(_mprime_in, streams,
>> data->k));
>> if (byte < 0) {
>> dev_dbg(dev, "mei_cldev_send failed. %zd\n", byte);
>> return byte;
>>
>> struct_size(_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?
>>
>
> This is even better:
>
> diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c
> b/drivers/misc/mei/hdcp/mei_hdcp.c
> index e6c3dc595617..852374565cba 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)));
> + flex_array_size(_mprime_in, streams, data->k));
>
> verify_mprime_in.k = cpu_to_be16(data->k);
>
> byte = mei_cldev_send(cldev, (u8 *)_mprime_in,
> - sizeof(verify_mprime_in));
> + struct_size(_mprime_in, streams,
> data->k));
> if (byte < 0) {
> dev_dbg(dev, "mei_cldev_send failed. %zd\n", byte);
> return byte;
>
> the flex_array_size() is a new helper that was designed for this situations.
> :)
> It calculates the size of a flexible array member within an enclosing
> structure,
> which is exactly the case.
>
> Thanks
> --
> Gustavo
>
>> 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
Re: [char-misc-next] Revert "mei: hdcp: Replace one-element array with flexible-array member"
On 7/28/20 18:04, Gustavo A. R. Silva wrote:
> 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 *)_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 *)_mprime_in,
> - sizeof(verify_mprime_in));
> + struct_size(_mprime_in, streams,
> data->k));
> if (byte < 0) {
> dev_dbg(dev, "mei_cldev_send failed. %zd\n", byte);
> return byte;
>
> struct_size(_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?
>
This is even better:
diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
index e6c3dc595617..852374565cba 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)));
+ flex_array_size(_mprime_in, streams, data->k));
verify_mprime_in.k = cpu_to_be16(data->k);
byte = mei_cldev_send(cldev, (u8 *)_mprime_in,
- sizeof(verify_mprime_in));
+ struct_size(_mprime_in, streams, data->k));
if (byte < 0) {
dev_dbg(dev, "mei_cldev_send failed. %zd\n", byte);
return byte;
the flex_array_size() is a new helper that was designed for this situations. :)
It calculates the size of a flexible array member within an enclosing structure,
which is exactly the case.
Thanks
--
Gustavo
> 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
(supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman
(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
(supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman
(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
>>> L: linux-kernel@vger.kernel.org
>>> S: Supported
>>> F:
RE: [char-misc-next] Revert "mei: hdcp: Replace one-element array with flexible-array member"
> On Tue, 2020-07-28 at 22:14 +, Winkler, Tomas wrote: > > > -Original Message- > > > From: Gustavo A. R. Silva > [] > > > 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 > > > (supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman > > > (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 > > > (supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman > > > (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/* ? > > No, this pattern matches _only_ files in that particular directory and not any > files in any subdirectory. > > > Maybe it is not recursive? > > It depends on the pattern. > Patterns with trailing slashes match subdirectories too. > > from MAINTAINERS: > > F: *Files* and directories wildcard patterns. > A trailing slash includes all files and subdirectory files. > F: drivers/net/all files in and below drivers/net > F: drivers/net/* all files in drivers/net, but not below > F: */net/* all files in "any top level directory"/net > One pattern per line. Multiple F: lines acceptable. > > > > Need to check the script, > > frankly I usually check this manually. prob > > > > INTEL MANAGEMENT ENGINE (mei) > > M: Tomas Winkler > > L: linux-kernel@vger.kernel.org > > S: Supported > > F: Documentation/driver-api/mei/* > > F: drivers/misc/mei/* > > You probably want: > > F:drivers/misc/mei/ Yes, thanks. Tomas
RE: [char-misc-next] Revert "mei: hdcp: Replace one-element array with flexible-array member"
>
> 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 *)_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 *)_mprime_in,
> - sizeof(verify_mprime_in));
> + struct_size(_mprime_in, streams,
> + data->k));
> if (byte < 0) {
> dev_dbg(dev, "mei_cldev_send failed. %zd\n", byte);
> return byte;
>
> struct_size(_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
> >>> (supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman
> >>> (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
> >>> (supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman
> >>> (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
> >> 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
> >> Signed-off-by: Tomas Winkler
> >> ---
> >> 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
>
Re: [char-misc-next] Revert "mei: hdcp: Replace one-element array with flexible-array member"
On Tue, 2020-07-28 at 22:14 +, Winkler, Tomas wrote:
> > -Original Message-
> > From: Gustavo A. R. Silva
[]
> > 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
> > (supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman
> > (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
> > (supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman
> > (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/* ?
No, this pattern matches _only_ files in that
particular directory and not any files in any
subdirectory.
> Maybe it is not recursive?
It depends on the pattern.
Patterns with trailing slashes match subdirectories too.
from MAINTAINERS:
F: *Files* and directories wildcard patterns.
A trailing slash includes all files and subdirectory files.
F: drivers/net/all files in and below drivers/net
F: drivers/net/* all files in drivers/net, but not below
F: */net/* all files in "any top level directory"/net
One pattern per line. Multiple F: lines acceptable.
> Need to check the script,
> frankly I usually check this manually. prob
>
> INTEL MANAGEMENT ENGINE (mei)
> M: Tomas Winkler
> L: linux-kernel@vger.kernel.org
> S: Supported
> F: Documentation/driver-api/mei/*
> F: drivers/misc/mei/*
You probably want:
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/*
>
>
> > 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
> > > > > Signed-off-by: Tomas Winkler
> > > > > ---
> > > > > 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 {
> > > > >
Re: [char-misc-next] Revert "mei: hdcp: Replace one-element array with flexible-array member"
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 *)_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 *)_mprime_in,
- sizeof(verify_mprime_in));
+ struct_size(_mprime_in, streams, data->k));
if (byte < 0) {
dev_dbg(dev, "mei_cldev_send failed. %zd\n", byte);
return byte;
struct_size(_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?
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
>>> (supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman
>>> (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
>>> (supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman
>>> (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
>> 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
>> Signed-off-by: Tomas Winkler
>> ---
>> 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);
>>
RE: [char-misc-next] Revert "mei: hdcp: Replace one-element array with flexible-array member"
> > >>
> > >> 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 *)_mprime_in,
> > > sizeof(verify_mprime_in));
> > >
> >
> > 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
> > (supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman
> > (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
> > (supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman
> > (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
> 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.
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
> > >>> Signed-off-by: Tomas Winkler
> > >>> ---
> > >>> 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 {
> > >>>
RE: [char-misc-next] Revert "mei: hdcp: Replace one-element array with flexible-array member"
> -Original Message-
> From: Gustavo A. R. Silva
> Sent: Wednesday, July 29, 2020 01:15
> To: Winkler, Tomas ; Greg Kroah-Hartman
>
> Cc: Usyskin, Alexander ; linux-
> ker...@vger.kernel.org; Gustavo A . R . Silva
> Subject: Re: [char-misc-next] Revert "mei: hdcp: Replace one-element array
> with flexible-array member"
>
>
>
> On 7/28/20 17:01, Winkler, Tomas wrote:
> >
> >
> >> -Original Message-
> >> From: Gustavo A. R. Silva
> >> Sent: Wednesday, July 29, 2020 00:57
> >> To: Winkler, Tomas ; Greg Kroah-Hartman
> >>
> >> Cc: Usyskin, Alexander ; linux-
> >> ker...@vger.kernel.org; Gustavo A . R . Silva
> >> Subject: Re: [char-misc-next] Revert "mei: hdcp: Replace one-element
> >> array with flexible-array member"
> >>
> >> 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 *)_mprime_in,
> > sizeof(verify_mprime_in));
> >
>
> 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
> (supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman
> (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
> (supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman
> (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
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/*
>
> 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
> >>> Signed-off-by: Tomas Winkler
> >>> ---
> >>> 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 {
> >>>
Re: [char-misc-next] Revert "mei: hdcp: Replace one-element array with flexible-array member"
On 7/28/20 17:14, Gustavo A. R. Silva wrote:
>
>
> On 7/28/20 17:01, Winkler, Tomas wrote:
>>
>>
>>> -Original Message-
>>> From: Gustavo A. R. Silva
>>> Sent: Wednesday, July 29, 2020 00:57
>>> To: Winkler, Tomas ; Greg Kroah-Hartman
>>>
>>> Cc: Usyskin, Alexander ; linux-
>>> ker...@vger.kernel.org; Gustavo A . R . Silva
>>> Subject: Re: [char-misc-next] Revert "mei: hdcp: Replace one-element array
>>> with flexible-array member"
>>>
>>> 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 *)_mprime_in,
>> sizeof(verify_mprime_in));
>>
>
> 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 (supporter:CHAR and MISC DRIVERS)
> Greg Kroah-Hartman (supporter:CHAR and MISC
> DRIVERS)
> linux-kernel@vger.kernel.org (open list)
> $ scripts/get_maintainer.pl --nokeywords --nogit --nogit-fallback
> drivers/misc/mei/hdcp/mei_hdcp.h
> Arnd Bergmann (supporter:CHAR and MISC DRIVERS)
> Greg Kroah-Hartman (supporter:CHAR and MISC
> DRIVERS)
> linux-kernel@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?>
Notice that if I run this command:
$ scripts/get_maintainer.pl -f drivers/misc/mei/hdcp/mei_hdcp.h
Arnd Bergmann (supporter:CHAR and MISC DRIVERS)
Greg Kroah-Hartman (supporter:CHAR and MISC
DRIVERS)
Ramalingam C
(commit_signer:2/2=100%,authored:2/2=100%,added_lines:4/4=100%,removed_lines:13/13=100%)
Tomas Winkler (commit_signer:2/2=100%)
Uma Shankar (commit_signer:2/2=100%)
Jani Nikula (commit_signer:2/2=100%)
Shashank Sharma (commit_signer:2/2=100%)
linux-kernel@vger.kernel.org (open list)
You appear this time, but not as a maintainer.
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
>>>> Signed-off-by: Tomas Winkler
>>>> ---
>>>> 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 {
>>>>
Re: [char-misc-next] Revert "mei: hdcp: Replace one-element array with flexible-array member"
On 7/28/20 17:01, Winkler, Tomas wrote:
>
>
>> -Original Message-
>> From: Gustavo A. R. Silva
>> Sent: Wednesday, July 29, 2020 00:57
>> To: Winkler, Tomas ; Greg Kroah-Hartman
>>
>> Cc: Usyskin, Alexander ; linux-
>> ker...@vger.kernel.org; Gustavo A . R . Silva
>> Subject: Re: [char-misc-next] Revert "mei: hdcp: Replace one-element array
>> with flexible-array member"
>>
>> 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 *)_mprime_in,
> sizeof(verify_mprime_in));
>
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 (supporter:CHAR and MISC DRIVERS)
Greg Kroah-Hartman (supporter:CHAR and MISC
DRIVERS)
linux-kernel@vger.kernel.org (open list)
$ scripts/get_maintainer.pl --nokeywords --nogit --nogit-fallback
drivers/misc/mei/hdcp/mei_hdcp.h
Arnd Bergmann (supporter:CHAR and MISC DRIVERS)
Greg Kroah-Hartman (supporter:CHAR and MISC
DRIVERS)
linux-kernel@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?
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
>>> Signed-off-by: Tomas Winkler
>>> ---
>>> 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 {
>>>
RE: [char-misc-next] Revert "mei: hdcp: Replace one-element array with flexible-array member"
> -Original Message-
> From: Gustavo A. R. Silva
> Sent: Wednesday, July 29, 2020 00:57
> To: Winkler, Tomas ; Greg Kroah-Hartman
>
> Cc: Usyskin, Alexander ; linux-
> ker...@vger.kernel.org; Gustavo A . R . Silva
> Subject: Re: [char-misc-next] Revert "mei: hdcp: Replace one-element array
> with flexible-array member"
>
> 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 *)_mprime_in,
sizeof(verify_mprime_in));
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.
>
> 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
> > Signed-off-by: Tomas Winkler
> > ---
> > 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 {
> >
Re: [char-misc-next] Revert "mei: hdcp: Replace one-element array with flexible-array member"
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?
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
> Signed-off-by: Tomas Winkler
> ---
> 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 {
>
