Re: [PATCH 2/2] x86: intel-mid: sfi_handle_*_dev() should check for pdata error code

[David Cohen <david.a.cohen@linux.intel.com>]

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 <david.a.cohen@linux.intel.com> wrote:
> > 
> > > Hi Ingo,
> > > 
> > > On Fri, Dec 20, 2013 at 09:49:53AM +0100, Ingo Molnar wrote:
> > > > 
> > > > * David Cohen <david.a.cohen@linux.intel.com> 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