Hi Ingo,
On Fri, Dec 20, 2013 at 09:49:53AM +0100, Ingo Molnar wrote:
>
> * David Cohen <[email protected]> wrote:
>
> > Prevent sfi_handle_*_dev() to register device in case
> > intel_mid_sfi_get_pdata() failed to execute.
> >
> > Since 'NULL' is a valid return value, this patch makes
> > sfi_handle_*_dev() functions to use IS_ERR() to validate returned pdata.
>
> Is this bug triggering in practice? If not then please say so in the
> changelog. If yes then is this patch desired for v3.13 merging and
> also please fix the changelog to conform to the standard changelog
> style:
>
> - first describe the symptoms of the bug - how does a user notice?
>
> - then describe how the code behaves today and how that is causing
> the bug
>
> - and then only describe how it's fixed.
>
> The first item is the most important one - while developers
> (naturally) tend to concentrate on the least important point, the last
> one.
Thanks for the feedback :)
This new patch set was done in reply to your comment:
https://lkml.org/lkml/2013/12/20/517
Br, David Cohen
>
> Thanks,
>
> Ingo
* David Cohen <[email protected]> wrote:
> Hi Ingo,
>
> On Fri, Dec 20, 2013 at 09:49:53AM +0100, Ingo Molnar wrote:
> >
> > * David Cohen <[email protected]> wrote:
> >
> > > Prevent sfi_handle_*_dev() to register device in case
> > > intel_mid_sfi_get_pdata() failed to execute.
> > >
> > > Since 'NULL' is a valid return value, this patch makes
> > > sfi_handle_*_dev() functions to use IS_ERR() to validate returned pdata.
> >
> > Is this bug triggering in practice? If not then please say so in the
> > changelog. If yes then is this patch desired for v3.13 merging and
> > also please fix the changelog to conform to the standard changelog
> > style:
> >
> > - first describe the symptoms of the bug - how does a user notice?
> >
> > - then describe how the code behaves today and how that is causing
> > the bug
> >
> > - and then only describe how it's fixed.
> >
> > The first item is the most important one - while developers
> > (naturally) tend to concentrate on the least important point, the last
> > one.
>
> Thanks for the feedback :)
> This new patch set was done in reply to your comment:
> https://lkml.org/lkml/2013/12/20/517
Hm, in what way does the new changelog address my first request:
> > - first describe the symptoms of the bug - how does a user notice?
They are all phrased as bug fixes, yet _none_ of the three changelogs
appears to describe specific symptoms on specific systems - they all
seem to talk in the abstract, with no specific connection to reality.
That really makes it harder for patches to get into the (way too
narrow) attention span of maintainersm, while phrasing it like this:
'If an Intel-MID system boots in a specific SFI environment then it
will hang on bootup without this fix.'
or:
'Existing Intel-MID hardware will run faster with this patch.'
will certainly wake up maintainers like a good coffee in the morning.
If a patch is a cleanup with no known bug fix effects then say so in
the title and the changelog.
Thanks,
Ingo
On Wed, Jan 15, 2014 at 07:58:37AM +0100, Ingo Molnar wrote:
>
> * David Cohen <[email protected]> wrote:
>
> > Hi Ingo,
> >
> > On Fri, Dec 20, 2013 at 09:49:53AM +0100, Ingo Molnar wrote:
> > >
> > > * David Cohen <[email protected]> wrote:
> > >
> > > > Prevent sfi_handle_*_dev() to register device in case
> > > > intel_mid_sfi_get_pdata() failed to execute.
> > > >
> > > > Since 'NULL' is a valid return value, this patch makes
> > > > sfi_handle_*_dev() functions to use IS_ERR() to validate returned pdata.
> > >
> > > Is this bug triggering in practice? If not then please say so in the
> > > changelog. If yes then is this patch desired for v3.13 merging and
> > > also please fix the changelog to conform to the standard changelog
> > > style:
> > >
> > > - first describe the symptoms of the bug - how does a user notice?
> > >
> > > - then describe how the code behaves today and how that is causing
> > > the bug
> > >
> > > - and then only describe how it's fixed.
> > >
> > > The first item is the most important one - while developers
> > > (naturally) tend to concentrate on the least important point, the last
> > > one.
> >
> > Thanks for the feedback :)
> > This new patch set was done in reply to your comment:
> > https://lkml.org/lkml/2013/12/20/517
>
> Hm, in what way does the new changelog address my first request:
>
> > > - first describe the symptoms of the bug - how does a user notice?
>
> They are all phrased as bug fixes, yet _none_ of the three changelogs
> appears to describe specific symptoms on specific systems - they all
> seem to talk in the abstract, with no specific connection to reality.
>
> That really makes it harder for patches to get into the (way too
> narrow) attention span of maintainersm, while phrasing it like this:
>
> 'If an Intel-MID system boots in a specific SFI environment then it
> will hang on bootup without this fix.'
>
> or:
>
> 'Existing Intel-MID hardware will run faster with this patch.'
>
> will certainly wake up maintainers like a good coffee in the morning.
>
> If a patch is a cleanup with no known bug fix effects then say so in
> the title and the changelog.
Fair enough.
These patches are fixing a potential bug that exists in current kernel,
but I triggered with patches in my development tree that depends on
this one to be refactored first:
https://patchwork.kernel.org/patch/3109791/
I tried to describe the potential bug, but it lacks the real use case as
you pointed out. I'll resend the patches in a way to trigger and
describe the situation without dependiing on non-upstreamed patches yet.
And I'll hurry up to publish my intel mid devel tree as well.
I hope the new patch set tastes like good morning Brazilian coffee :)
Br, David Cohen
>
> Thanks,
>
> Ingo
Hi Ingo and hpa,
On Wed, Jan 15, 2014 at 09:39:52AM -0800, David Cohen wrote:
> On Wed, Jan 15, 2014 at 07:58:37AM +0100, Ingo Molnar wrote:
> >
> > * David Cohen <[email protected]> wrote:
> >
> > > Hi Ingo,
> > >
> > > On Fri, Dec 20, 2013 at 09:49:53AM +0100, Ingo Molnar wrote:
> > > >
> > > > * David Cohen <[email protected]> wrote:
> > > >
> > > > > Prevent sfi_handle_*_dev() to register device in case
> > > > > intel_mid_sfi_get_pdata() failed to execute.
> > > > >
> > > > > Since 'NULL' is a valid return value, this patch makes
> > > > > sfi_handle_*_dev() functions to use IS_ERR() to validate returned pdata.
> > > >
> > > > Is this bug triggering in practice? If not then please say so in the
> > > > changelog. If yes then is this patch desired for v3.13 merging and
> > > > also please fix the changelog to conform to the standard changelog
> > > > style:
> > > >
> > > > - first describe the symptoms of the bug - how does a user notice?
> > > >
> > > > - then describe how the code behaves today and how that is causing
> > > > the bug
> > > >
> > > > - and then only describe how it's fixed.
> > > >
> > > > The first item is the most important one - while developers
> > > > (naturally) tend to concentrate on the least important point, the last
> > > > one.
> > >
> > > Thanks for the feedback :)
> > > This new patch set was done in reply to your comment:
> > > https://lkml.org/lkml/2013/12/20/517
> >
> > Hm, in what way does the new changelog address my first request:
> >
> > > > - first describe the symptoms of the bug - how does a user notice?
> >
> > They are all phrased as bug fixes, yet _none_ of the three changelogs
> > appears to describe specific symptoms on specific systems - they all
> > seem to talk in the abstract, with no specific connection to reality.
> >
> > That really makes it harder for patches to get into the (way too
> > narrow) attention span of maintainersm, while phrasing it like this:
> >
> > 'If an Intel-MID system boots in a specific SFI environment then it
> > will hang on bootup without this fix.'
> >
> > or:
> >
> > 'Existing Intel-MID hardware will run faster with this patch.'
> >
> > will certainly wake up maintainers like a good coffee in the morning.
> >
> > If a patch is a cleanup with no known bug fix effects then say so in
> > the title and the changelog.
>
> Fair enough.
> These patches are fixing a potential bug that exists in current kernel,
> but I triggered with patches in my development tree that depends on
> this one to be refactored first:
> https://patchwork.kernel.org/patch/3109791/
>
> I tried to describe the potential bug, but it lacks the real use case as
> you pointed out. I'll resend the patches in a way to trigger and
> describe the situation without dependiing on non-upstreamed patches yet.
> And I'll hurry up to publish my intel mid devel tree as well.
>
> I hope the new patch set tastes like good morning Brazilian coffee :)
In order to show a practical error case fixed by this patch set using
current legacy platform code, I need to get them working first. But it
turns out legacy platform code (for Moorestown and Medfield) aren't in a
good shape at all. I found few cases of obsolete platform data being
returned from platform code (intel mid was orphan for too long on
upstream).
I'll have to append new patches to this set "[PATCH v2 0/3] x86:
intel-mid: handle platform code error in better way", so it won't be a
simple fix of patch description.
In order to not block the rest of my patches on thread "[PATCH v2 0/4]
Add Clovertrail and Merrifeld support to Intel MID", please consider to
apply them first (maybe for 3.14 if possible).
When I resend these patches here, we can consider apply them on 3.14-rcX
(as they are bug fixes) or just postpone them to >3.14.
Thanks,
David Cohen
>
> Br, David Cohen
>
> >
> > Thanks,
> >
> > Ingo
* David Cohen <[email protected]> wrote:
> Hi Ingo and hpa,
>
> On Wed, Jan 15, 2014 at 09:39:52AM -0800, David Cohen wrote:
> > On Wed, Jan 15, 2014 at 07:58:37AM +0100, Ingo Molnar wrote:
> > >
> > > * David Cohen <[email protected]> wrote:
> > >
> > > > Hi Ingo,
> > > >
> > > > On Fri, Dec 20, 2013 at 09:49:53AM +0100, Ingo Molnar wrote:
> > > > >
> > > > > * David Cohen <[email protected]> wrote:
> > > > >
> > > > > > Prevent sfi_handle_*_dev() to register device in case
> > > > > > intel_mid_sfi_get_pdata() failed to execute.
> > > > > >
> > > > > > Since 'NULL' is a valid return value, this patch makes
> > > > > > sfi_handle_*_dev() functions to use IS_ERR() to validate returned pdata.
> > > > >
> > > > > Is this bug triggering in practice? If not then please say so in the
> > > > > changelog. If yes then is this patch desired for v3.13 merging and
> > > > > also please fix the changelog to conform to the standard changelog
> > > > > style:
> > > > >
> > > > > - first describe the symptoms of the bug - how does a user notice?
> > > > >
> > > > > - then describe how the code behaves today and how that is causing
> > > > > the bug
> > > > >
> > > > > - and then only describe how it's fixed.
> > > > >
> > > > > The first item is the most important one - while developers
> > > > > (naturally) tend to concentrate on the least important point, the last
> > > > > one.
> > > >
> > > > Thanks for the feedback :)
> > > > This new patch set was done in reply to your comment:
> > > > https://lkml.org/lkml/2013/12/20/517
> > >
> > > Hm, in what way does the new changelog address my first request:
> > >
> > > > > - first describe the symptoms of the bug - how does a user notice?
> > >
> > > They are all phrased as bug fixes, yet _none_ of the three changelogs
> > > appears to describe specific symptoms on specific systems - they all
> > > seem to talk in the abstract, with no specific connection to reality.
> > >
> > > That really makes it harder for patches to get into the (way too
> > > narrow) attention span of maintainersm, while phrasing it like this:
> > >
> > > 'If an Intel-MID system boots in a specific SFI environment then it
> > > will hang on bootup without this fix.'
> > >
> > > or:
> > >
> > > 'Existing Intel-MID hardware will run faster with this patch.'
> > >
> > > will certainly wake up maintainers like a good coffee in the morning.
> > >
> > > If a patch is a cleanup with no known bug fix effects then say so in
> > > the title and the changelog.
> >
> > Fair enough.
> > These patches are fixing a potential bug that exists in current kernel,
> > but I triggered with patches in my development tree that depends on
> > this one to be refactored first:
> > https://patchwork.kernel.org/patch/3109791/
> >
> > I tried to describe the potential bug, but it lacks the real use case as
> > you pointed out. I'll resend the patches in a way to trigger and
> > describe the situation without dependiing on non-upstreamed patches yet.
> > And I'll hurry up to publish my intel mid devel tree as well.
> >
> > I hope the new patch set tastes like good morning Brazilian coffee :)
>
> In order to show a practical error case fixed by this patch set
> using current legacy platform code, I need to get them working
> first. But it turns out legacy platform code (for Moorestown and
> Medfield) aren't in a good shape at all. I found few cases of
> obsolete platform data being returned from platform code (intel mid
> was orphan for too long on upstream).
>
> I'll have to append new patches to this set "[PATCH v2 0/3] x86:
> intel-mid: handle platform code error in better way", so it won't be
> a simple fix of patch description.
Great, more fixes to the code is the best kind of fix to a changelog.
> In order to not block the rest of my patches on thread "[PATCH v2
> 0/4] Add Clovertrail and Merrifeld support to Intel MID", please
> consider to apply them first (maybe for 3.14 if possible).
Sure, those look fine to me, but please don't forget about these fixes
either.
> When I resend these patches here, we can consider apply them on
> 3.14-rcX (as they are bug fixes) or just postpone them to >3.14.
Please send them ASAP, don't wait for v3.14 -rc's. We'll handle the
logistics. Sending those 3 fixes with an improved changelog would be a
good start.
Thanks,
Ingo
On Thu, Jan 16, 2014 at 10:50:06AM +0100, Ingo Molnar wrote:
>
> * David Cohen <[email protected]> wrote:
>
> > Hi Ingo and hpa,
> >
> > On Wed, Jan 15, 2014 at 09:39:52AM -0800, David Cohen wrote:
> > > On Wed, Jan 15, 2014 at 07:58:37AM +0100, Ingo Molnar wrote:
> > > >
> > > > * David Cohen <[email protected]> wrote:
> > > >
> > > > > Hi Ingo,
> > > > >
> > > > > On Fri, Dec 20, 2013 at 09:49:53AM +0100, Ingo Molnar wrote:
> > > > > >
> > > > > > * David Cohen <[email protected]> wrote:
> > > > > >
> > > > > > > Prevent sfi_handle_*_dev() to register device in case
> > > > > > > intel_mid_sfi_get_pdata() failed to execute.
> > > > > > >
> > > > > > > Since 'NULL' is a valid return value, this patch makes
> > > > > > > sfi_handle_*_dev() functions to use IS_ERR() to validate returned pdata.
> > > > > >
> > > > > > Is this bug triggering in practice? If not then please say so in the
> > > > > > changelog. If yes then is this patch desired for v3.13 merging and
> > > > > > also please fix the changelog to conform to the standard changelog
> > > > > > style:
> > > > > >
> > > > > > - first describe the symptoms of the bug - how does a user notice?
> > > > > >
> > > > > > - then describe how the code behaves today and how that is causing
> > > > > > the bug
> > > > > >
> > > > > > - and then only describe how it's fixed.
> > > > > >
> > > > > > The first item is the most important one - while developers
> > > > > > (naturally) tend to concentrate on the least important point, the last
> > > > > > one.
> > > > >
> > > > > Thanks for the feedback :)
> > > > > This new patch set was done in reply to your comment:
> > > > > https://lkml.org/lkml/2013/12/20/517
> > > >
> > > > Hm, in what way does the new changelog address my first request:
> > > >
> > > > > > - first describe the symptoms of the bug - how does a user notice?
> > > >
> > > > They are all phrased as bug fixes, yet _none_ of the three changelogs
> > > > appears to describe specific symptoms on specific systems - they all
> > > > seem to talk in the abstract, with no specific connection to reality.
> > > >
> > > > That really makes it harder for patches to get into the (way too
> > > > narrow) attention span of maintainersm, while phrasing it like this:
> > > >
> > > > 'If an Intel-MID system boots in a specific SFI environment then it
> > > > will hang on bootup without this fix.'
> > > >
> > > > or:
> > > >
> > > > 'Existing Intel-MID hardware will run faster with this patch.'
> > > >
> > > > will certainly wake up maintainers like a good coffee in the morning.
> > > >
> > > > If a patch is a cleanup with no known bug fix effects then say so in
> > > > the title and the changelog.
> > >
> > > Fair enough.
> > > These patches are fixing a potential bug that exists in current kernel,
> > > but I triggered with patches in my development tree that depends on
> > > this one to be refactored first:
> > > https://patchwork.kernel.org/patch/3109791/
> > >
> > > I tried to describe the potential bug, but it lacks the real use case as
> > > you pointed out. I'll resend the patches in a way to trigger and
> > > describe the situation without dependiing on non-upstreamed patches yet.
> > > And I'll hurry up to publish my intel mid devel tree as well.
> > >
> > > I hope the new patch set tastes like good morning Brazilian coffee :)
> >
> > In order to show a practical error case fixed by this patch set
> > using current legacy platform code, I need to get them working
> > first. But it turns out legacy platform code (for Moorestown and
> > Medfield) aren't in a good shape at all. I found few cases of
> > obsolete platform data being returned from platform code (intel mid
> > was orphan for too long on upstream).
> >
> > I'll have to append new patches to this set "[PATCH v2 0/3] x86:
> > intel-mid: handle platform code error in better way", so it won't be
> > a simple fix of patch description.
>
> Great, more fixes to the code is the best kind of fix to a changelog.
IMHO it's better than say "considering this driver works, it would fail
this way because of SFI doing that thing..."
But thinking twice, I'm tempted to say these patches simply don't do
functional changes right now. Than I can handle fixing the platform code
and the new driver's upstreaming that depends on this fix perhaps on 3.15.
>
> > In order to not block the rest of my patches on thread "[PATCH v2
> > 0/4] Add Clovertrail and Merrifeld support to Intel MID", please
> > consider to apply them first (maybe for 3.14 if possible).
>
> Sure, those look fine to me, but please don't forget about these fixes
> either.
Thanks.
>
> > When I resend these patches here, we can consider apply them on
> > 3.14-rcX (as they are bug fixes) or just postpone them to >3.14.
>
> Please send them ASAP, don't wait for v3.14 -rc's. We'll handle the
> logistics. Sending those 3 fixes with an improved changelog would be a
> good start.
I'm currently stuck with full day trainings for the rest of the week in
my job. I can resend ASAP if assuming this is a non-functional change at
this moment.
Br, David Cohen
>
> Thanks,
>
> Ingo
On Thu, Jan 16, 2014 at 09:23:54AM -0800, David Cohen wrote:
> On Thu, Jan 16, 2014 at 10:50:06AM +0100, Ingo Molnar wrote:
> >
> > * David Cohen <[email protected]> wrote:
> >
> > > Hi Ingo and hpa,
> > >
> > > On Wed, Jan 15, 2014 at 09:39:52AM -0800, David Cohen wrote:
> > > > On Wed, Jan 15, 2014 at 07:58:37AM +0100, Ingo Molnar wrote:
> > > > >
> > > > > * David Cohen <[email protected]> wrote:
> > > > >
> > > > > > Hi Ingo,
> > > > > >
> > > > > > On Fri, Dec 20, 2013 at 09:49:53AM +0100, Ingo Molnar wrote:
> > > > > > >
> > > > > > > * David Cohen <[email protected]> wrote:
> > > > > > >
> > > > > > > > Prevent sfi_handle_*_dev() to register device in case
> > > > > > > > intel_mid_sfi_get_pdata() failed to execute.
> > > > > > > >
> > > > > > > > Since 'NULL' is a valid return value, this patch makes
> > > > > > > > sfi_handle_*_dev() functions to use IS_ERR() to validate returned pdata.
> > > > > > >
> > > > > > > Is this bug triggering in practice? If not then please say so in the
> > > > > > > changelog. If yes then is this patch desired for v3.13 merging and
> > > > > > > also please fix the changelog to conform to the standard changelog
> > > > > > > style:
> > > > > > >
> > > > > > > - first describe the symptoms of the bug - how does a user notice?
> > > > > > >
> > > > > > > - then describe how the code behaves today and how that is causing
> > > > > > > the bug
> > > > > > >
> > > > > > > - and then only describe how it's fixed.
> > > > > > >
> > > > > > > The first item is the most important one - while developers
> > > > > > > (naturally) tend to concentrate on the least important point, the last
> > > > > > > one.
> > > > > >
> > > > > > Thanks for the feedback :)
> > > > > > This new patch set was done in reply to your comment:
> > > > > > https://lkml.org/lkml/2013/12/20/517
> > > > >
> > > > > Hm, in what way does the new changelog address my first request:
> > > > >
> > > > > > > - first describe the symptoms of the bug - how does a user notice?
> > > > >
> > > > > They are all phrased as bug fixes, yet _none_ of the three changelogs
> > > > > appears to describe specific symptoms on specific systems - they all
> > > > > seem to talk in the abstract, with no specific connection to reality.
> > > > >
> > > > > That really makes it harder for patches to get into the (way too
> > > > > narrow) attention span of maintainersm, while phrasing it like this:
> > > > >
> > > > > 'If an Intel-MID system boots in a specific SFI environment then it
> > > > > will hang on bootup without this fix.'
> > > > >
> > > > > or:
> > > > >
> > > > > 'Existing Intel-MID hardware will run faster with this patch.'
> > > > >
> > > > > will certainly wake up maintainers like a good coffee in the morning.
> > > > >
> > > > > If a patch is a cleanup with no known bug fix effects then say so in
> > > > > the title and the changelog.
> > > >
> > > > Fair enough.
> > > > These patches are fixing a potential bug that exists in current kernel,
> > > > but I triggered with patches in my development tree that depends on
> > > > this one to be refactored first:
> > > > https://patchwork.kernel.org/patch/3109791/
> > > >
> > > > I tried to describe the potential bug, but it lacks the real use case as
> > > > you pointed out. I'll resend the patches in a way to trigger and
> > > > describe the situation without dependiing on non-upstreamed patches yet.
> > > > And I'll hurry up to publish my intel mid devel tree as well.
> > > >
> > > > I hope the new patch set tastes like good morning Brazilian coffee :)
> > >
> > > In order to show a practical error case fixed by this patch set
> > > using current legacy platform code, I need to get them working
> > > first. But it turns out legacy platform code (for Moorestown and
> > > Medfield) aren't in a good shape at all. I found few cases of
> > > obsolete platform data being returned from platform code (intel mid
> > > was orphan for too long on upstream).
> > >
> > > I'll have to append new patches to this set "[PATCH v2 0/3] x86:
> > > intel-mid: handle platform code error in better way", so it won't be
> > > a simple fix of patch description.
> >
> > Great, more fixes to the code is the best kind of fix to a changelog.
>
> IMHO it's better than say "considering this driver works, it would fail
> this way because of SFI doing that thing..."
>
> But thinking twice, I'm tempted to say these patches simply don't do
> functional changes right now. Than I can handle fixing the platform code
> and the new driver's upstreaming that depends on this fix perhaps on 3.15.
>
> >
> > > In order to not block the rest of my patches on thread "[PATCH v2
> > > 0/4] Add Clovertrail and Merrifeld support to Intel MID", please
> > > consider to apply them first (maybe for 3.14 if possible).
> >
> > Sure, those look fine to me, but please don't forget about these fixes
> > either.
>
> Thanks.
>
> >
> > > When I resend these patches here, we can consider apply them on
> > > 3.14-rcX (as they are bug fixes) or just postpone them to >3.14.
> >
> > Please send them ASAP, don't wait for v3.14 -rc's. We'll handle the
> > logistics. Sending those 3 fixes with an improved changelog would be a
> > good start.
>
> I'm currently stuck with full day trainings for the rest of the week in
> my job. I can resend ASAP if assuming this is a non-functional change at
> this moment.
I resent this patch set here:
https://lkml.org/lkml/2014/1/16/498
It is non-functional now (explicitly said on changelogs).
I hope it's all good this time.
Br, David Cohen