Patch Question

Patch Question

[Perry Hooker]

Hi everyone,

I recently submitted a patch to the kernel mailing list:
https://lkml.org/lkml/2017/3/21/712

I received some feedback on the patch. After a bit of polite
back-and-forth, the respondent stopped replying when I asked for more
information, and I haven't heard anything from the maintainers.

Based on my analysis (contained in the thread), I still think the
patch is correct & appropriate.

What's the best way to determine if this is a good fix or not?
How should I proceed if the patch is, in fact, a good fix?

Best regards,
Perry

Patch Question

[Mandeep Sandhu]

On Mon, Apr 17, 2017 at 4:28 PM, Perry Hooker <perry.hooker@gmail.com>
wrote:

> Hi everyone,
>
> I recently submitted a patch to the kernel mailing list:
> https://lkml.org/lkml/2017/3/21/712


lkml.org seems to be down! Wow, what did you do!?! :P

-mandeep




>
>
> I received some feedback on the patch. After a bit of polite
> back-and-forth, the respondent stopped replying when I asked for more
> information, and I haven't heard anything from the maintainers.
>
> Based on my analysis (contained in the thread), I still think the
> patch is correct & appropriate.
>
> What's the best way to determine if this is a good fix or not?
> How should I proceed if the patch is, in fact, a good fix?
>
> Best regards,
> Perry
>
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies at kernelnewbies.org
> https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20170417/9dbcbe65/attachment.html 

Patch Question

[Tobin C. Harding]

On Mon, Apr 17, 2017 at 05:28:46PM -0600, Perry Hooker wrote:
> Hi everyone,
> 
> I recently submitted a patch to the kernel mailing list:
> https://lkml.org/lkml/2017/3/21/712

Link is broken.

> I received some feedback on the patch. After a bit of polite
> back-and-forth, the respondent stopped replying when I asked for more
> information, and I haven't heard anything from the maintainers.

No one *has* to respond to your email.

> Based on my analysis (contained in the thread), I still think the
> patch is correct & appropriate.

Perhaps you just need to rework it a bit as the reviewer suggested?

> What's the best way to determine if this is a good fix or not?
> How should I proceed if the patch is, in fact, a good fix?

If the patch was good it would have probably been picked up.

I have found myself in similar positions. Often, since we are just
beginners, there is some thing about the situation that we do not
fully understand. This lack of understanding leads us to think we are
correct when in fact we are not. Perhaps you could go back over the
reviewers emails and think all around the code being discussed, make
sure you understand every minute detail of what is being done.

I have found reviewers to be unusually patient with us newbies, if you
display that you have put in effort to try and understand their
position most times you will get a response. If you don't perhaps the
fix is not worth bothering with, the kernel is large there are always
more things to work on.

Hope this helps,
Tobin.

Patch Question

[Perry Hooker]

Thanks for the advice, Tobin - I appreciate the reply.

In this case, I've already followed your advice - I studied the
reviewer's comments with a fine-toothed comb (some of his comments
were flat-out incorrect), and traced the buffer in question back to
its source. It appears to be holding host-endian data, and it's being
cast to a little-endian type without an explicit conversion. The patch
I submitted fixes this by using the kernel-defined byte-order macros.

I've reached out to the reviewer both individually and via the mailing
list, and haven't heard back.

It's possible that I'm missing something, but I don't see what.

At what point is it appropriate to re-submit the patch?

Here's the link to the last message in the thread:
https://lkml.org/lkml/2017/4/10/1045

Best regards,
Perry


On Mon, Apr 17, 2017 at 5:58 PM, Tobin C. Harding <me@tobin.cc> wrote:
> On Mon, Apr 17, 2017 at 05:28:46PM -0600, Perry Hooker wrote:
>> Hi everyone,
>>
>> I recently submitted a patch to the kernel mailing list:
>> https://lkml.org/lkml/2017/3/21/712
>
> Link is broken.
>
>> I received some feedback on the patch. After a bit of polite
>> back-and-forth, the respondent stopped replying when I asked for more
>> information, and I haven't heard anything from the maintainers.
>
> No one *has* to respond to your email.
>
>> Based on my analysis (contained in the thread), I still think the
>> patch is correct & appropriate.
>
> Perhaps you just need to rework it a bit as the reviewer suggested?
>
>> What's the best way to determine if this is a good fix or not?
>> How should I proceed if the patch is, in fact, a good fix?
>
> If the patch was good it would have probably been picked up.
>
> I have found myself in similar positions. Often, since we are just
> beginners, there is some thing about the situation that we do not
> fully understand. This lack of understanding leads us to think we are
> correct when in fact we are not. Perhaps you could go back over the
> reviewers emails and think all around the code being discussed, make
> sure you understand every minute detail of what is being done.
>
> I have found reviewers to be unusually patient with us newbies, if you
> display that you have put in effort to try and understand their
> position most times you will get a response. If you don't perhaps the
> fix is not worth bothering with, the kernel is large there are always
> more things to work on.
>
> Hope this helps,
> Tobin.

Patch Question

[Tobin C. Harding]

On Tue, Apr 18, 2017 at 05:07:08PM -0600, Perry Hooker wrote:
> Thanks for the advice, Tobin - I appreciate the reply.

Please don't top post http://daringfireball.net/2007/07/on_top

I'm not an endian expert so I will not comment on the technical
aspects of the path, I can however, comment on the thread and why you
may not be getting the response you desire.

> In this case, I've already followed your advice - I studied the
> reviewer's comments with a fine-toothed comb (some of his comments
> were flat-out incorrect)

Dan Carpenter is very good at what he dose. I would be hesitant to
ever call him or anyone as experienced 'flat-out incorrect'.

> , and traced the buffer in question back to
> its source. It appears to be holding host-endian data, and it's being
> cast to a little-endian type without an explicit conversion. The patch
> I submitted fixes this by using the kernel-defined byte-order macros.

His initial reply hints that this patch may need testing before it can
be applied - have you tested the patch on real hardware? If so, and it
is correct, re-submit the patch stating so.

> I've reached out to the reviewer both individually and via the mailing
> list, and haven't heard back.

>From the thread, and this is only my opinion, it seems Dan has put
more effort than is required of him already. No one is paid to answer
questions on LKML, they are not required to apply effort to your
problems, anything they do is a gift of their time and should be appreciated.

> It's possible that I'm missing something, but I don't see what.

In your comments you in no way display that you understand exactly
what the code is doing and why it should by changed. Your initial
patch does not have an appropriate changelog message, please read
Documentation/process/submitting-patches.rst (section 2 Describing
your changes).

A more subtle point - you may have more success if you do not put
demands onto people (eg can you explain this..) but rather write out
your understanding of the code explaining why your hold the views you
do. Others can then comment or this, agreeing or disagreeing as the
case may be. People like to help by giving their knowledge, no one
likes doing chores.

> At what point is it appropriate to re-submit the patch?

Once you have reworked the patch, taken into consideration the
reviewers comments, written a changelog describing the code as it is
and why it needs changing, explained the patch fully so that the previous
reviewer and future reviewers can understand that you understand what
is going on. Then it is appropriate to re-submit the patch ([PATCH v2]...).

Again, I am only new around here, these are my opinions based on what
I have seen and read. By no means should they be taken as gospel.

Remember, we are all here to make the kernel better. It's not
personal, it's about the kernel.

I hope this helps, best of luck.

Tobin.

Patch Question

[Perry Hooker]

> Dan Carpenter is very good at what he dose. I would be hesitant to
> ever call him or anyone as experienced 'flat-out incorrect'.

Mr. Carpenter's first assertion that the patch "introduces bugs" was
incorrect by his own admission:
https://lkml.org/lkml/2017/3/22/120

Additionally, his assertion that the "buff likely is just a regular
ieee80211_hdr struct" was also incorrect.

> In your comments you in no way display that you understand exactly
> what the code is doing and why it should by changed. Your initial
> patch does not have an appropriate changelog message, please read
> Documentation/process/submitting-patches.rst (section 2 Describing
> your changes).

I disagree. I've written both where the buffer is allocated and where
it's filled, and why the contents need to be converted to
little-endian byte order. Can you comment on whether my analysis is
correct?

Also, here are several commits that fix similar issues with similar
changelog messages:
https://github.com/torvalds/linux/commit/a15505e69cd2f8d0ebf566cd5c5838bd5c2d56e3
https://github.com/torvalds/linux/commit/47910a49db876397150b9754bc66f0c179448854
https://github.com/torvalds/linux/commit/af27e01cfcfcdf7f45488e023b474eb6de5f732e

Can you comment on what's considered an "appropriate changelog message?"

> A more subtle point - you may have more success if you do not put
> demands onto people (eg can you explain this..) but rather write out
> your understanding of the code explaining why your hold the views you
> do. Others can then comment or this, agreeing or disagreeing as the
> case may be. People like to help by giving their knowledge, no one
> likes doing chores.

As I mentioned (and is contained in the thread), I've written out my
understanding of where the buffer is allocated, where it's filled, and
why the conversion needs to happen.
At this point, no one has commented on the accuracy of my analysis.
Such comments are exactly what I'm hoping for (and what I requested -
not "demanded" - in my replies).

So, if you can provide some useful analysis, I'd be happy to listen.

Patch Question

[Greg KH]

On Wed, Apr 19, 2017 at 09:54:33AM -0600, Perry Hooker wrote:
> At this point, no one has commented on the accuracy of my analysis.
> Such comments are exactly what I'm hoping for (and what I requested -
> not "demanded" - in my replies).

As the one responsible for actually applying this specific patch, I find
this pretty "interesting"...

It's _your_ responsibility to convince me that your patch is correct and
should be applied.  If no one respondes after a week or so, resend it,
with your additional information in the changelog so that the same
conversation doesn't happen again.

Patches get dropped all the time, remember, I get _hundreds_ of them
every day.  If I see a disagreement on a patch, then it goes to the end
of the line, or usually just deleted from my queue.  I rely on the fact
that it's up to the submitter to resend and handle the discussion.

Yes, we waste engineering time and effort by doing this, and that's
fine, we have thousands of developers, but very few maintainers.  It's
their time that is valuable and the process is optimized for.

good luck!

greg k-h

Patch Question

[Perry Hooker]

Thank you for your reply.

> It's _your_ responsibility to convince me that your patch is correct and
> should be applied.  If no one respondes after a week or so, resend it,
> with your additional information in the changelog so that the same
> conversation doesn't happen again.

Understood. I'm still open to the possibility that I've made a mistake
- I don't want to re-submit the patch if my analysis is incorrect.

Maybe I didn't make it clear (my apologies if so) - what I'm really
looking for here is help confirming or refuting my work. If
re-submitting the patch is the best way to do this, then I can
certainly go that route.

Regards,
Perry

Patch Question

[Lino Sanfilippo]

On 19.04.2017 18:46, Perry Hooker wrote:
> 
> Understood. I'm still open to the possibility that I've made a mistake
> - I don't want to re-submit the patch if my analysis is incorrect.
> 
> Maybe I didn't make it clear (my apologies if so) - what I'm really
> looking for here is help confirming or refuting my work. If
> re-submitting the patch is the best way to do this, then I can
> certainly go that route.
> 

Before you do this you should double check that the raised objections are indeed not justified. 
I have not looked too deep into the code but in function WILC_WFI_p2p_rx() the buffer 
is conditionally passed to cfg80211_rx_mgmt() which handles the passed data as being little endian.
To me this is a strong indication that the data in the buffer is also little endian (which is what
Dan pointed out and why the change you propose is not correct).

Regards,
Lino

Patch Question

[Lino Sanfilippo]

Hi,

On 18.04.2017 01:28, Perry Hooker wrote:
> Hi everyone,
> 
> I recently submitted a patch to the kernel mailing list:
> https://lkml.org/lkml/2017/3/21/712
> 
> I received some feedback on the patch. After a bit of polite
> back-and-forth, the respondent stopped replying when I asked for more
> information, and I haven't heard anything from the maintainers.
> 
> Based on my analysis (contained in the thread), I still think the
> patch is correct & appropriate.

As far as I understood Dan Carpenters (last) post in that thread, the content in the buffer
is already in little endian order. In this case the code is correct as it is and there is no need 
for the change you propose.


Regards,
Lino

Patch Question

[Perry Hooker]

> As far as I understood Dan Carpenters (last) post in that thread, the content in the buffer
> is already in little endian order. In this case the code is correct as it is and there is no need
> for the change you propose.

Yes, I believe Mr. Carpenter is mistaken - I think the data is in
host-endian order.

Please see my analysis of where that buffer's contents are filled. Can
you comment on whether my analysis is correct or not?

Regards,
Perry