On 17/10/2023 11:18, Tylor Yang wrote:
> Hello,
>
> This patch series adds the driver for Himax HID-over-SPI touchscreen ICs.
> This driver takes a position in [1], it intends to take advantage of SPI
> transfer speed and HID interface.
>
Dear Google/Chromium folks,
As a multi-billion company I am sure you can spare some small amount of
time/effort/money for internal review before using community for this
purpose. I mean reviewing trivial issues, like coding style, or just
running checkpatch. You know, the obvious things.
There is no need to use expensive time of community reviewers to review
very simple mistakes, the ones which we fixed in Linux kernel years ago
(also with automated tools). You can and you should do it, before
submitting drivers for community review.
Thanks in advance.
Best regards,
Krzysztof
Hi,
On Tue, Oct 17, 2023 at 10:08 AM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 17/10/2023 11:18, Tylor Yang wrote:
> > Hello,
> >
> > This patch series adds the driver for Himax HID-over-SPI touchscreen ICs.
> > This driver takes a position in [1], it intends to take advantage of SPI
> > transfer speed and HID interface.
> >
>
> Dear Google/Chromium folks,
>
> As a multi-billion company I am sure you can spare some small amount of
> time/effort/money for internal review before using community for this
> purpose. I mean reviewing trivial issues, like coding style, or just
> running checkpatch. You know, the obvious things.
>
> There is no need to use expensive time of community reviewers to review
> very simple mistakes, the ones which we fixed in Linux kernel years ago
> (also with automated tools). You can and you should do it, before
> submitting drivers for community review.
We can certainly talk more about this, but a quick reply is:
1. If a patch really looks super bad to you then the right thing for
you to do is to respond to the patch with some canned response saying
"you didn't even do these basic things--please read the documentation
and work with someone at Google to get a basic review". This seems
like a perfectly legit response and I don't think you should do more
than that.
2. IMO as a general rule "internal review" should be considered
harmful. When you're a new submitter then absolutely you should get
some internal review from someone who has done this before, but making
"internal review" a requirement for all patches leads to frustration
all around. It leads to people redesigning their code in response to
"internal review" and then getting frustrated when external
maintainers tell them to do something totally different. ...then
upstream reviewers respond to the frustration with "Why were you
designing your code behind closed doors? If you had done the review in
the public and on the mailing lists then someone could have stopped
you before you changed everything".
3. The ChromeOS team is organized much more like the upstream
community than a big hierarchical corporation. Just as it's not easy
for you to control the behavior of other maintainers, it is not
trivial for one person on the team to control what others on the team
will do. We could make an attempt to institute rules like "all patches
must go through internal review before being posted", but as per #2 I
don't think this is a good idea. The ChromeOS team has even less
control over what our partners may or may not do. In general it is
always a struggle to get partners to even start working upstream and
IMO it's a win when I see a partner post a patch. We should certainly
help partners be successful here, but the right way to do that is by
offering them support.
About the best we can do is to provide good documentation for people
learning how to send patches. Right now the ChromeOS kernel docs [1]
suggest using "patman" to send patches and I have seen many partners
do this. Patman will, at the very least, run checkpatch for you. Our
instructions also say that you should make sure you run "checkpatch"
yourself if you don't run patman. If people aren't following these
docs that we already have then there's not much we can do.
So I guess the tl;dr from my side:
a) People should absolutely be posting on mailing lists and not (as a
rule) doing "internal review".
b) If a patch looks really broken to you, don't get upset and don't
waste your time. Just respond and say that you'll look at it once it
looks better and suggest that they get a review (preferably on the
mailing lists!) from someone they're working with at Google.
https://chromium.googlesource.com/chromiumos/docs/+/HEAD/kernel_development.md#send-out-the-patch-using-patman
-Doug
On 17/10/2023 23:41, Doug Anderson wrote:
> Hi,
>
> On Tue, Oct 17, 2023 at 10:08 AM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 17/10/2023 11:18, Tylor Yang wrote:
>>> Hello,
>>>
>>> This patch series adds the driver for Himax HID-over-SPI touchscreen ICs.
>>> This driver takes a position in [1], it intends to take advantage of SPI
>>> transfer speed and HID interface.
>>>
>>
>> Dear Google/Chromium folks,
>>
>> As a multi-billion company I am sure you can spare some small amount of
>> time/effort/money for internal review before using community for this
>> purpose. I mean reviewing trivial issues, like coding style, or just
>> running checkpatch. You know, the obvious things.
>>
>> There is no need to use expensive time of community reviewers to review
>> very simple mistakes, the ones which we fixed in Linux kernel years ago
>> (also with automated tools). You can and you should do it, before
>> submitting drivers for community review.
>
> We can certainly talk more about this, but a quick reply is:
>
> 1. If a patch really looks super bad to you then the right thing for
> you to do is to respond to the patch with some canned response saying
> "you didn't even do these basic things--please read the documentation
> and work with someone at Google to get a basic review". This seems
> like a perfectly legit response and I don't think you should do more
> than that.
>
> 2. IMO as a general rule "internal review" should be considered
> harmful. When you're a new submitter then absolutely you should get
> some internal review from someone who has done this before, but making
> "internal review" a requirement for all patches leads to frustration
> all around. It leads to people redesigning their code in response to
> "internal review" and then getting frustrated when external
> maintainers tell them to do something totally different. ...then
> upstream reviewers respond to the frustration with "Why were you
> designing your code behind closed doors? If you had done the review in
> the public and on the mailing lists then someone could have stopped
> you before you changed everything".
No one expects forced internal review on mature contributions. We talk
here about a first time contribution where already basic mistakes were
made: like not using get_maintainers.pl, not using checkpatch, not using
other tools and finally sending code which does not look like Linux
kernel code at all.
>
> 3. The ChromeOS team is organized much more like the upstream
> community than a big hierarchical corporation. Just as it's not easy
> for you to control the behavior of other maintainers, it is not
> trivial for one person on the team to control what others on the team
> will do. We could make an attempt to institute rules like "all patches
> must go through internal review before being posted", but as per #2 I
> don't think this is a good idea. The ChromeOS team has even less
> control over what our partners may or may not do. In general it is
> always a struggle to get partners to even start working upstream and
> IMO it's a win when I see a partner post a patch. We should certainly
> help partners be successful here, but the right way to do that is by
> offering them support.
I don't know who is exactly core team, who is partner. I see
"google.com" domain, so Google folks are responsible for not wasting
time of the community. If Google disagrees, please change the domain so
I will understand that and not feel like Google wants to use us all. I
am fine and I understand if small companies or individuals make such
mistakes. It feels like a waste of our time if Google makes such
mistakes. Google's (Alphabet's) revenue for 2022 was 282 billions USD
and net revenue was 59 billions USD.
>
> About the best we can do is to provide good documentation for people
> learning how to send patches. Right now the ChromeOS kernel docs [1]
> suggest using "patman" to send patches and I have seen many partners
> do this. Patman will, at the very least, run checkpatch for you. Our
> instructions also say that you should make sure you run "checkpatch"
> yourself if you don't run patman. If people aren't following these
> docs that we already have then there's not much we can do.
>
>
> So I guess the tl;dr from my side:
>
> a) People should absolutely be posting on mailing lists and not (as a
> rule) doing "internal review".
>
> b) If a patch looks really broken to you, don't get upset and don't
> waste your time. Just respond and say that you'll look at it once it
> looks better and suggest that they get a review (preferably on the
> mailing lists!) from someone they're working with at Google.
Best regards,
Krzysztof
Hi Doug, Krzysztof,
On Wed, Oct 18, 2023 at 8:07 AM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 17/10/2023 23:41, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Oct 17, 2023 at 10:08 AM Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 17/10/2023 11:18, Tylor Yang wrote:
> >>> Hello,
> >>>
> >>> This patch series adds the driver for Himax HID-over-SPI touchscreen ICs.
> >>> This driver takes a position in [1], it intends to take advantage of SPI
> >>> transfer speed and HID interface.
> >>>
> >>
> >> Dear Google/Chromium folks,
> >>
> >> As a multi-billion company I am sure you can spare some small amount of
> >> time/effort/money for internal review before using community for this
> >> purpose. I mean reviewing trivial issues, like coding style, or just
> >> running checkpatch. You know, the obvious things.
> >>
> >> There is no need to use expensive time of community reviewers to review
> >> very simple mistakes, the ones which we fixed in Linux kernel years ago
> >> (also with automated tools). You can and you should do it, before
> >> submitting drivers for community review.
> >
> > We can certainly talk more about this, but a quick reply is:
> >
> > 1. If a patch really looks super bad to you then the right thing for
> > you to do is to respond to the patch with some canned response saying
> > "you didn't even do these basic things--please read the documentation
> > and work with someone at Google to get a basic review". This seems
> > like a perfectly legit response and I don't think you should do more
> > than that.
As explained below, the point here is not to do an internal review to
come up with a final patch that has been signed-off by everybody. The
point is to have a minimum of hand holding when a submission looks
terrible.
We have seen a bunch of (small) patches that have multiple
signed-off-by when they are submitted for the first time, and it's
perfectly fine. It often leads to a fast forward inclusion process.
But in that particular scenario, the patches could have looked less
rude toward the community by having a first internal swipe to say:
- please run checkpatch before submitting
- please use kernel coding style
- please do not send a 9000 loc single patch (it got slightly better
in v3, but still is way too big for anybody to make a correct review
IMO).
And if you prefer to do these things openly, I would have expected
someone from Google to have at least said that publicly.
> >
> > 2. IMO as a general rule "internal review" should be considered
> > harmful. When you're a new submitter then absolutely you should get
> > some internal review from someone who has done this before, but making
> > "internal review" a requirement for all patches leads to frustration
> > all around. It leads to people redesigning their code in response to
> > "internal review" and then getting frustrated when external
> > maintainers tell them to do something totally different. ...then
> > upstream reviewers respond to the frustration with "Why were you
> > designing your code behind closed doors? If you had done the review in
> > the public and on the mailing lists then someone could have stopped
> > you before you changed everything".
>
> No one expects forced internal review on mature contributions. We talk
> here about a first time contribution where already basic mistakes were
> made: like not using get_maintainers.pl, not using checkpatch, not using
> other tools and finally sending code which does not look like Linux
> kernel code at all.
Basically, what we want is a little bit of mentoring: "can I send this
to the community? - well, there are obvious things that will be a
problem".
>
> >
> > 3. The ChromeOS team is organized much more like the upstream
> > community than a big hierarchical corporation. Just as it's not easy
> > for you to control the behavior of other maintainers, it is not
> > trivial for one person on the team to control what others on the team
> > will do. We could make an attempt to institute rules like "all patches
> > must go through internal review before being posted", but as per #2 I
> > don't think this is a good idea. The ChromeOS team has even less
> > control over what our partners may or may not do. In general it is
> > always a struggle to get partners to even start working upstream and
> > IMO it's a win when I see a partner post a patch. We should certainly
> > help partners be successful here, but the right way to do that is by
> > offering them support.
>
> I don't know who is exactly core team, who is partner. I see
> "google.com" domain, so Google folks are responsible for not wasting
> time of the community. If Google disagrees, please change the domain so
> I will understand that and not feel like Google wants to use us all. I
> am fine and I understand if small companies or individuals make such
> mistakes. It feels like a waste of our time if Google makes such
> mistakes. Google's (Alphabet's) revenue for 2022 was 282 billions USD
> and net revenue was 59 billions USD.
>
> >
> > About the best we can do is to provide good documentation for people
> > learning how to send patches. Right now the ChromeOS kernel docs [1]
> > suggest using "patman" to send patches and I have seen many partners
> > do this. Patman will, at the very least, run checkpatch for you. Our
> > instructions also say that you should make sure you run "checkpatch"
> > yourself if you don't run patman. If people aren't following these
> > docs that we already have then there's not much we can do.
> >
> >
> > So I guess the tl;dr from my side:
> >
> > a) People should absolutely be posting on mailing lists and not (as a
> > rule) doing "internal review".
> >
> > b) If a patch looks really broken to you, don't get upset and don't
> > waste your time. Just respond and say that you'll look at it once it
> > looks better and suggest that they get a review (preferably on the
> > mailing lists!) from someone they're working with at Google.
The problem is that the form of the very first submission looked
terrible, with a single patch of 9000 changes. It was even rejected by
the mailing list.
I think I understand why this happened, but if that patch is not split
in functional simple logic blocks, there is no way we are going to
review and merge this.
Anyway, many thanks to Krzysztof for trying to review the code, which
I dropped already from v1.
Cheers,
Benjamin
On Wed, Oct 18, 2023 at 08:07:32AM +0200, Krzysztof Kozlowski wrote:
> On 17/10/2023 23:41, Doug Anderson wrote:
> >
> > 3. The ChromeOS team is organized much more like the upstream
> > community than a big hierarchical corporation. Just as it's not easy
> > for you to control the behavior of other maintainers, it is not
> > trivial for one person on the team to control what others on the team
> > will do. We could make an attempt to institute rules like "all patches
> > must go through internal review before being posted", but as per #2 I
> > don't think this is a good idea. The ChromeOS team has even less
> > control over what our partners may or may not do. In general it is
> > always a struggle to get partners to even start working upstream and
> > IMO it's a win when I see a partner post a patch. We should certainly
> > help partners be successful here, but the right way to do that is by
> > offering them support.
>
> I don't know who is exactly core team, who is partner. I see
> "google.com" domain, so Google folks are responsible for not wasting
> time of the community. If Google disagrees, please change the domain so
> I will understand that and not feel like Google wants to use us all.
I think it might help if you think of <company>.corp-partner.google.com
addresses as gmail.com addresses. People who are using these addresses
are not employees of Google nor contractors for Google; they work for
their respective <company>.
The main reason person@<company>.corp-partner.google.com addresses are
being used for mainline submissions is because it is actually possible
to set up "git send-email" with them, as their main domain typically
handled by Exchange and mandates Outlook.
Thanks.
--
Dmitry
Hi Krzysztof,
On Wed, Oct 18, 2023 at 2:08 AM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 17/10/2023 11:18, Tylor Yang wrote:
> > Hello,
> >
> > This patch series adds the driver for Himax HID-over-SPI touchscreen ICs.
> > This driver takes a position in [1], it intends to take advantage of SPI
> > transfer speed and HID interface.
> >
>
> Dear Google/Chromium folks,
>
> As a multi-billion company I am sure you can spare some small amount of
> time/effort/money for internal review before using community for this
> purpose. I mean reviewing trivial issues, like coding style, or just
> running checkpatch. You know, the obvious things.
>
> There is no need to use expensive time of community reviewers to review
> very simple mistakes, the ones which we fixed in Linux kernel years ago
> (also with automated tools). You can and you should do it, before
> submitting drivers for community review.
>
> Thanks in advance.
First of all, I can understand your sentiment towards some of the
patches being in a very rough shape. As a community we have large
volumes of patches to review and it would be really helpful if new
contributors followed some basic simple steps, as described in our
"Submitting patches" page...
That said, it's not a fair assumption that there are no steps taken to
offload the upstream reviewers community by the corporate
contributors. We usually do have basic internal pre-reviews for
patches coming from partners and even a pre-review bot (CoP) that can
automate some of the checks such as checkpatch or bisectability. But
as others said in this thread, we don't control our partners and they
are free to send the patches just directly to the mailing lists if
they want to do so. In a similar way, not everyone in ChromeOS is
super experienced with upstream submissions, so sometimes they may not
be aware of the best practices, etc.
I haven't seen the patch in question, but I'd assume it's more like an
exception rather than a usual pattern, so I'd appreciate it if we
could avoid aggressive responses like that and try to solve the
problems in a more productive way. Just a simple response with a link
to https://www.kernel.org/doc/html/latest/process/submitting-patches.html
wouldn't really cost you much, or actually even less than the entire
litany in this email.
Let's be nice to each other. Thanks.
Best regards,
Tomasz
On 22/01/2024 05:57, Tomasz Figa wrote:
> Hi Krzysztof,
>
> On Wed, Oct 18, 2023 at 2:08 AM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 17/10/2023 11:18, Tylor Yang wrote:
>>> Hello,
>>>
>>> This patch series adds the driver for Himax HID-over-SPI touchscreen ICs.
>>> This driver takes a position in [1], it intends to take advantage of SPI
>>> transfer speed and HID interface.
>>>
>>
>> Dear Google/Chromium folks,
>>
>> As a multi-billion company I am sure you can spare some small amount of
>> time/effort/money for internal review before using community for this
>> purpose. I mean reviewing trivial issues, like coding style, or just
>> running checkpatch. You know, the obvious things.
>>
>> There is no need to use expensive time of community reviewers to review
>> very simple mistakes, the ones which we fixed in Linux kernel years ago
>> (also with automated tools). You can and you should do it, before
>> submitting drivers for community review.
>>
>> Thanks in advance.
>
> First of all, I can understand your sentiment towards some of the
> patches being in a very rough shape. As a community we have large
> volumes of patches to review and it would be really helpful if new
> contributors followed some basic simple steps, as described in our
> "Submitting patches" page...
I don't really understand why responding to something which is three
months old. Anyway, I talked with Doug on Plumbers about it so things
are more or less clarified, however since two Google folks responded,
let me continue.
>
> That said, it's not a fair assumption that there are no steps taken to
> offload the upstream reviewers community by the corporate
> contributors. We usually do have basic internal pre-reviews for
> patches coming from partners and even a pre-review bot (CoP) that can
Good to know.
> automate some of the checks such as checkpatch or bisectability. But
> as others said in this thread, we don't control our partners and they
> are free to send the patches just directly to the mailing lists if
> they want to do so. In a similar way, not everyone in ChromeOS is
> super experienced with upstream submissions, so sometimes they may not
> be aware of the best practices, etc.
>
> I haven't seen the patch in question, but I'd assume it's more like an
> exception rather than a usual pattern, so I'd appreciate it if we
Unfortunately that's the pattern. I was complaining few times about very
poor quality of some patches from some partners before writing that email.
Just to clarify: all the complains are about missing basic stuff, like
running basic tools. They don't even require internal review by humans.
> could avoid aggressive responses like that and try to solve the
> problems in a more productive way. Just a simple response with a link
> to https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> wouldn't really cost you much, or actually even less than the entire
> litany in this email.
Simple response to docs don't work. Docs are quite long and contributors
questioned here just don't read them in details.
Best regards,
Krzysztof
On Mon, Jan 22, 2024 at 5:08 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 22/01/2024 05:57, Tomasz Figa wrote:
> > Hi Krzysztof,
> >
> > On Wed, Oct 18, 2023 at 2:08 AM Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 17/10/2023 11:18, Tylor Yang wrote:
> >>> Hello,
> >>>
> >>> This patch series adds the driver for Himax HID-over-SPI touchscreen ICs.
> >>> This driver takes a position in [1], it intends to take advantage of SPI
> >>> transfer speed and HID interface.
> >>>
> >>
> >> Dear Google/Chromium folks,
> >>
> >> As a multi-billion company I am sure you can spare some small amount of
> >> time/effort/money for internal review before using community for this
> >> purpose. I mean reviewing trivial issues, like coding style, or just
> >> running checkpatch. You know, the obvious things.
> >>
> >> There is no need to use expensive time of community reviewers to review
> >> very simple mistakes, the ones which we fixed in Linux kernel years ago
> >> (also with automated tools). You can and you should do it, before
> >> submitting drivers for community review.
> >>
> >> Thanks in advance.
> >
> > First of all, I can understand your sentiment towards some of the
> > patches being in a very rough shape. As a community we have large
> > volumes of patches to review and it would be really helpful if new
> > contributors followed some basic simple steps, as described in our
> > "Submitting patches" page...
>
> I don't really understand why responding to something which is three
> months old.
Uh, I got the reply from Dmitry 3 days ago and didn't realize it was that old.
> Anyway, I talked with Doug on Plumbers about it so things
> are more or less clarified, however since two Google folks responded,
> let me continue.
>
> >
> > That said, it's not a fair assumption that there are no steps taken to
> > offload the upstream reviewers community by the corporate
> > contributors. We usually do have basic internal pre-reviews for
> > patches coming from partners and even a pre-review bot (CoP) that can
>
> Good to know.
>
> > automate some of the checks such as checkpatch or bisectability. But
> > as others said in this thread, we don't control our partners and they
> > are free to send the patches just directly to the mailing lists if
> > they want to do so. In a similar way, not everyone in ChromeOS is
> > super experienced with upstream submissions, so sometimes they may not
> > be aware of the best practices, etc.
> >
> > I haven't seen the patch in question, but I'd assume it's more like an
> > exception rather than a usual pattern, so I'd appreciate it if we
>
> Unfortunately that's the pattern. I was complaining few times about very
> poor quality of some patches from some partners before writing that email.
>
> Just to clarify: all the complains are about missing basic stuff, like
> running basic tools. They don't even require internal review by humans.
>
Hmm, that's sad then, but then also as I said, we don't control our
partners, so we can't really guarantee that every single patch goes
through some kind of internal review. Hopefully this has improved by
now and continues to improve as the tooling I mentioned gets more
widely used.
> > could avoid aggressive responses like that and try to solve the
> > problems in a more productive way. Just a simple response with a link
> > to https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> > wouldn't really cost you much, or actually even less than the entire
> > litany in this email.
>
> Simple response to docs don't work. Docs are quite long and contributors
> questioned here just don't read them in details.
If asking someone to read a doc doesn't lead to that person reading
the doc, then that's another story. My personal experience is
different, though... But well, again, we're talking about humans, so
we're going to end up with a wide range of extreme cases, regardless
of what we do.
Tbh. I (as a kernel maintainer too) would really imagine we also have
some more automated tooling for the upstream kernel workflows. I don't
see anything preventing some kind of checkpatch (or whatnot) bots,
responding to patches automatically. (Actually we're working on
something like this for the Media subsystem...)
Best regards,
Tomasz