[PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Dongsheng Wang]

From: Wang Dongsheng <dongsheng.wang@freescale.com>

If diu_ops is not implemented on platform, kernel will access a null
pointer. we need to check this pointer in diu initialization.

Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>

diff --git a/drivers/video/fbdev/fsl-diu-fb.c b/drivers/video/fbdev/fsl-diu-fb.c
index b335c1a..288b5e4 100644
--- a/drivers/video/fbdev/fsl-diu-fb.c
+++ b/drivers/video/fbdev/fsl-diu-fb.c
@@ -479,7 +479,10 @@ static enum fsl_diu_monitor_port fsl_diu_name_to_port(const char *s)
 			port = FSL_DIU_PORT_DLVDS;
 	}
 
-	return diu_ops.valid_monitor_port(port);
+	if (diu_ops.valid_monitor_port)
+		port = diu_ops.valid_monitor_port(port);
+
+	return port;
 }
 
 /*
@@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct platform_device *pdev)
 	unsigned int i;
 	int ret;
 
+	if (!diu_ops.set_pixel_clock)
+		return -ENODEV;
+
 	data = dmam_alloc_coherent(&pdev->dev, sizeof(struct fsl_diu_data),
 				   &dma_addr, GFP_DMA | __GFP_ZERO);
 	if (!data)
-- 
2.1.0.27.g96db324

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]



On 24/11/15 08:27, Dongsheng Wang wrote:
> From: Wang Dongsheng <dongsheng.wang@freescale.com>
> 
> If diu_ops is not implemented on platform, kernel will access a null
> pointer. we need to check this pointer in diu initialization.
> 
> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> 
> diff --git a/drivers/video/fbdev/fsl-diu-fb.c b/drivers/video/fbdev/fsl-diu-fb.c
> index b335c1a..288b5e4 100644
> --- a/drivers/video/fbdev/fsl-diu-fb.c
> +++ b/drivers/video/fbdev/fsl-diu-fb.c
> @@ -479,7 +479,10 @@ static enum fsl_diu_monitor_port fsl_diu_name_to_port(const char *s)
>  			port = FSL_DIU_PORT_DLVDS;
>  	}
>  
> -	return diu_ops.valid_monitor_port(port);
> +	if (diu_ops.valid_monitor_port)
> +		port = diu_ops.valid_monitor_port(port);
> +
> +	return port;
>  }
>  
>  /*
> @@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct platform_device *pdev)
>  	unsigned int i;
>  	int ret;
>  
> +	if (!diu_ops.set_pixel_clock)
> +		return -ENODEV;
> +
>  	data = dmam_alloc_coherent(&pdev->dev, sizeof(struct fsl_diu_data),
>  				   &dma_addr, GFP_DMA | __GFP_ZERO);
>  	if (!data)
> 

Thanks, queued for 4.5.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Wang Dongsheng]

VGhhbmtzIFRvbWkuDQoNClJlZ2FyZHMsDQotRG9uZ3NoZW5nDQoNCj4gDQo+IE9uIDI0LzExLzE1
IDA4OjI3LCBEb25nc2hlbmcgV2FuZyB3cm90ZToNCj4gPiBGcm9tOiBXYW5nIERvbmdzaGVuZyA8
ZG9uZ3NoZW5nLndhbmdAZnJlZXNjYWxlLmNvbT4NCj4gPg0KPiA+IElmIGRpdV9vcHMgaXMgbm90
IGltcGxlbWVudGVkIG9uIHBsYXRmb3JtLCBrZXJuZWwgd2lsbCBhY2Nlc3MgYSBudWxsDQo+ID4g
cG9pbnRlci4gd2UgbmVlZCB0byBjaGVjayB0aGlzIHBvaW50ZXIgaW4gZGl1IGluaXRpYWxpemF0
aW9uLg0KPiA+DQo+ID4gU2lnbmVkLW9mZi1ieTogV2FuZyBEb25nc2hlbmcgPGRvbmdzaGVuZy53
YW5nQGZyZWVzY2FsZS5jb20+DQo+ID4NCj4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy92aWRlby9m
YmRldi9mc2wtZGl1LWZiLmMNCj4gPiBiL2RyaXZlcnMvdmlkZW8vZmJkZXYvZnNsLWRpdS1mYi5j
DQo+ID4gaW5kZXggYjMzNWMxYS4uMjg4YjVlNCAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL3Zp
ZGVvL2ZiZGV2L2ZzbC1kaXUtZmIuYw0KPiA+ICsrKyBiL2RyaXZlcnMvdmlkZW8vZmJkZXYvZnNs
LWRpdS1mYi5jDQo+ID4gQEAgLTQ3OSw3ICs0NzksMTAgQEAgc3RhdGljIGVudW0gZnNsX2RpdV9t
b25pdG9yX3BvcnQNCj4gZnNsX2RpdV9uYW1lX3RvX3BvcnQoY29uc3QgY2hhciAqcykNCj4gPiAg
CQkJcG9ydCA9IEZTTF9ESVVfUE9SVF9ETFZEUzsNCj4gPiAgCX0NCj4gPg0KPiA+IC0JcmV0dXJu
IGRpdV9vcHMudmFsaWRfbW9uaXRvcl9wb3J0KHBvcnQpOw0KPiA+ICsJaWYgKGRpdV9vcHMudmFs
aWRfbW9uaXRvcl9wb3J0KQ0KPiA+ICsJCXBvcnQgPSBkaXVfb3BzLnZhbGlkX21vbml0b3JfcG9y
dChwb3J0KTsNCj4gPiArDQo+ID4gKwlyZXR1cm4gcG9ydDsNCj4gPiAgfQ0KPiA+DQo+ID4gIC8q
DQo+ID4gQEAgLTE2OTcsNiArMTcwMCw5IEBAIHN0YXRpYyBpbnQgZnNsX2RpdV9wcm9iZShzdHJ1
Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2KQ0KPiA+ICAJdW5zaWduZWQgaW50IGk7DQo+ID4gIAlp
bnQgcmV0Ow0KPiA+DQo+ID4gKwlpZiAoIWRpdV9vcHMuc2V0X3BpeGVsX2Nsb2NrKQ0KPiA+ICsJ
CXJldHVybiAtRU5PREVWOw0KPiA+ICsNCj4gPiAgCWRhdGEgPSBkbWFtX2FsbG9jX2NvaGVyZW50
KCZwZGV2LT5kZXYsIHNpemVvZihzdHJ1Y3QgZnNsX2RpdV9kYXRhKSwNCj4gPiAgCQkJCSAgICZk
bWFfYWRkciwgR0ZQX0RNQSB8IF9fR0ZQX1pFUk8pOw0KPiA+ICAJaWYgKCFkYXRhKQ0KPiA+DQo+
IA0KPiBUaGFua3MsIHF1ZXVlZCBmb3IgNC41Lg0KPiANCj4gIFRvbWkNCg0K

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Timur Tabi]

On Tue, Nov 24, 2015 at 5:46 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 24/11/15 08:27, Dongsheng Wang wrote:
>> @@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct platform_device *pdev)
>>       unsigned int i;
>>       int ret;
>>
>> +     if (!diu_ops.set_pixel_clock)
>> +             return -ENODEV;
>> +
>>       data = dmam_alloc_coherent(&pdev->dev, sizeof(struct fsl_diu_data),
>>                                  &dma_addr, GFP_DMA | __GFP_ZERO);
>>       if (!data)
>>
>
> Thanks, queued for 4.5.

Could you please wait for me to review the patch first?  I am the
maintainer for the driver, and I see a problem with it.

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Timur Tabi]

 On Tue, Nov 24, 2015 at 1:27 AM, Dongsheng Wang
<dongsheng.wang@freescale.com> wrote:
> @@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct platform_device *pdev)
>         unsigned int i;
>         int ret;
>
> +       if (!diu_ops.set_pixel_clock)
> +               return -ENODEV;
> +
>         data = dmam_alloc_coherent(&pdev->dev, sizeof(struct fsl_diu_data),
>                                    &dma_addr, GFP_DMA | __GFP_ZERO);
>         if (!data)

This doesn't make any sense.  If set_pixel_clock() is not defined,
then the whole driver aborts the probe.  When could that ever happen?
If the platform code does not exist, then don't let the driver be
probed.

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Scott Wood]

On Tue, 2015-11-24 at 11:12 -0500, Timur Tabi wrote:
>  On Tue, Nov 24, 2015 at 1:27 AM, Dongsheng Wang
> <dongsheng.wang@freescale.com> wrote:
> > @@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct platform_device
> > *pdev)
> >         unsigned int i;
> >         int ret;
> > 
> > +       if (!diu_ops.set_pixel_clock)
> > +               return -ENODEV;
> > +
> >         data = dmam_alloc_coherent(&pdev->dev, sizeof(struct
> > fsl_diu_data),
> >                                    &dma_addr, GFP_DMA | __GFP_ZERO);
> >         if (!data)
> 
> This doesn't make any sense.  If set_pixel_clock() is not defined,
> then the whole driver aborts the probe.

That's what this patch is trying to accomplish.  Currently it crashes instead.

>   When could that ever happen?
> If the platform code does not exist, then don't let the driver be
> probed.

How do you propose to accomplish that other than with such a check?

-Scott

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]



On 24/11/15 18:04, Timur Tabi wrote:
> On Tue, Nov 24, 2015 at 5:46 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On 24/11/15 08:27, Dongsheng Wang wrote:
>>> @@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct platform_device *pdev)
>>>       unsigned int i;
>>>       int ret;
>>>
>>> +     if (!diu_ops.set_pixel_clock)
>>> +             return -ENODEV;
>>> +
>>>       data = dmam_alloc_coherent(&pdev->dev, sizeof(struct fsl_diu_data),
>>>                                  &dma_addr, GFP_DMA | __GFP_ZERO);
>>>       if (!data)
>>>
>>
>> Thanks, queued for 4.5.
> 
> Could you please wait for me to review the patch first?  I am the
> maintainer for the driver, and I see a problem with it.

Sorry, I was too hasty (and tired). I thought I was looking at a patch
that's been on the list for a while, but apparently it was only posted
today...

Anyway, dropped this.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Timur Tabi]

On Tue, Nov 24, 2015 at 11:15 AM, Scott Wood <scottwood@freescale.com> wrote:
> On Tue, 2015-11-24 at 11:12 -0500, Timur Tabi wrote:
>>  On Tue, Nov 24, 2015 at 1:27 AM, Dongsheng Wang
>> <dongsheng.wang@freescale.com> wrote:
>> > @@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct platform_device
>> > *pdev)
>> >         unsigned int i;
>> >         int ret;
>> >
>> > +       if (!diu_ops.set_pixel_clock)
>> > +               return -ENODEV;
>> > +
>> >         data = dmam_alloc_coherent(&pdev->dev, sizeof(struct
>> > fsl_diu_data),
>> >                                    &dma_addr, GFP_DMA | __GFP_ZERO);
>> >         if (!data)
>>
>> This doesn't make any sense.  If set_pixel_clock() is not defined,
>> then the whole driver aborts the probe.
>
> That's what this patch is trying to accomplish.  Currently it crashes instead.
>
>>   When could that ever happen?
>> If the platform code does not exist, then don't let the driver be
>> probed.
>
> How do you propose to accomplish that other than with such a check?

Well, if you're concern is that there's no platform code, then there
should be a check that says, "see if there's any platform code", not
"let's check this obscure function and abort without explanation if
it's not initialized."

Alternatively, why can't you just do this, in update_lcdc():




>
> -Scott
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Scott Wood]

On Tue, 2015-11-24 at 11:54 -0500, Timur Tabi wrote:
> On Tue, Nov 24, 2015 at 11:15 AM, Scott Wood <scottwood@freescale.com>
> wrote:
> > On Tue, 2015-11-24 at 11:12 -0500, Timur Tabi wrote:
> > >  On Tue, Nov 24, 2015 at 1:27 AM, Dongsheng Wang
> > > <dongsheng.wang@freescale.com> wrote:
> > > > @@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct platform_device
> > > > *pdev)
> > > >         unsigned int i;
> > > >         int ret;
> > > > 
> > > > +       if (!diu_ops.set_pixel_clock)
> > > > +               return -ENODEV;
> > > > +
> > > >         data = dmam_alloc_coherent(&pdev->dev, sizeof(struct
> > > > fsl_diu_data),
> > > >                                    &dma_addr, GFP_DMA | __GFP_ZERO);
> > > >         if (!data)
> > > 
> > > This doesn't make any sense.  If set_pixel_clock() is not defined,
> > > then the whole driver aborts the probe.
> > 
> > That's what this patch is trying to accomplish.  Currently it crashes
> > instead.
> > 
> > >   When could that ever happen?
> > > If the platform code does not exist, then don't let the driver be
> > > probed.
> > 
> > How do you propose to accomplish that other than with such a check?
> 
> Well, if you're concern is that there's no platform code, then there
> should be a check that says, "see if there's any platform code", not
> "let's check this obscure function and abort without explanation if
> it's not initialized."

Do you have a *specific* better way to "see if there's any platform code"?

> 
> Alternatively, why can't you just do this, in update_lcdc():
> 
> 
> 

Do nothing?

-Scott

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Timur Tabi]

On Tue, Nov 24, 2015 at 11:54 AM, Timur Tabi <timur@tabi.org> wrote:
> Well, if you're concern is that there's no platform code, then there
> should be a check that says, "see if there's any platform code", not
> "let's check this obscure function and abort without explanation if
> it's not initialized."
>
> Alternatively, why can't you just do this, in update_lcdc():

[Stupid gmail sent my message before I was done typing]

if (diu_ops.set_pixel_clock)
   diu_ops.set_pixel_clock(var->pixclock);

RE: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Wang Dongsheng]

SGkgVGltdXIsDQoNClRoYW5rcyBmb3IgeW91ciByZXZpZXcuDQoNCj4gIE9uIFR1ZSwgTm92IDI0
LCAyMDE1IGF0IDE6MjcgQU0sIERvbmdzaGVuZyBXYW5nDQo+IDxkb25nc2hlbmcud2FuZ0BmcmVl
c2NhbGUuY29tPiB3cm90ZToNCj4gPiBAQCAtMTY5Nyw2ICsxNzAwLDkgQEAgc3RhdGljIGludCBm
c2xfZGl1X3Byb2JlKHN0cnVjdCBwbGF0Zm9ybV9kZXZpY2UgKnBkZXYpDQo+ID4gICAgICAgICB1
bnNpZ25lZCBpbnQgaTsNCj4gPiAgICAgICAgIGludCByZXQ7DQo+ID4NCj4gPiArICAgICAgIGlm
ICghZGl1X29wcy5zZXRfcGl4ZWxfY2xvY2spDQo+ID4gKyAgICAgICAgICAgICAgIHJldHVybiAt
RU5PREVWOw0KPiA+ICsNCj4gPiAgICAgICAgIGRhdGEgPSBkbWFtX2FsbG9jX2NvaGVyZW50KCZw
ZGV2LT5kZXYsIHNpemVvZihzdHJ1Y3QgZnNsX2RpdV9kYXRhKSwNCj4gPiAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICZkbWFfYWRkciwgR0ZQX0RNQSB8IF9fR0ZQX1pFUk8pOw0K
PiA+ICAgICAgICAgaWYgKCFkYXRhKQ0KPiANCj4gVGhpcyBkb2Vzbid0IG1ha2UgYW55IHNlbnNl
LiAgSWYgc2V0X3BpeGVsX2Nsb2NrKCkgaXMgbm90IGRlZmluZWQsDQo+IHRoZW4gdGhlIHdob2xl
IGRyaXZlciBhYm9ydHMgdGhlIHByb2JlLiAgV2hlbiBjb3VsZCB0aGF0IGV2ZXIgaGFwcGVuPw0K
PiBJZiB0aGUgcGxhdGZvcm0gY29kZSBkb2VzIG5vdCBleGlzdCwgdGhlbiBkb24ndCBsZXQgdGhl
IGRyaXZlciBiZQ0KPiBwcm9iZWQuDQoNCkFub3RoZXIgcGF0Y2ggdmlkZW86IGZiZGV2OiBmc2w6
IFNwbGl0IERJVSBpbml0aWFsaXphdGlvbiBlbnRyeTpbaHR0cHM6Ly9wYXRjaHdvcmsua2VybmVs
Lm9yZy9wYXRjaC83MzgxMzUxL10NCm1vZHVsZV9pbml0KGZzbF9kaXVfaW5pdCkgd2lsbCBiZSBy
ZXBsYWNlLCBhbmQgYWxsIG9mIHRoZSBpbml0aWFsaXphdGlvbiB3aWxsIGJlIGNvbXBsZXRlZCBp
biB0aGUgcHJvYmUNCmluY2x1ZGUgdGhpcyBjaGVjay4gU28ganVzdCBkbyBhIHF1aWNrIGZpeCBm
b3IgdGhpcyBib290IGtlcm5lbCBjcmFzaCBpc3N1ZS4NCg0KUmVnYXJkcywNCi1Eb25nc2hlbmcN
Cg=

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Scott Wood]

On Tue, 2015-11-24 at 18:15 +0200, Tomi Valkeinen wrote:
> 
> On 24/11/15 18:04, Timur Tabi wrote:
> > On Tue, Nov 24, 2015 at 5:46 AM, Tomi Valkeinen <tomi.valkeinen@ti.com>
> > wrote:
> > > On 24/11/15 08:27, Dongsheng Wang wrote:
> > > > @@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct platform_device
> > > > *pdev)
> > > >       unsigned int i;
> > > >       int ret;
> > > > 
> > > > +     if (!diu_ops.set_pixel_clock)
> > > > +             return -ENODEV;
> > > > +
> > > >       data = dmam_alloc_coherent(&pdev->dev, sizeof(struct
> > > > fsl_diu_data),
> > > >                                  &dma_addr, GFP_DMA | __GFP_ZERO);
> > > >       if (!data)
> > > > 
> > > 
> > > Thanks, queued for 4.5.
> > 
> > Could you please wait for me to review the patch first?  I am the
> > maintainer for the driver, and I see a problem with it.
> 
> Sorry, I was too hasty (and tired). I thought I was looking at a patch
> that's been on the list for a while, but apparently it was only posted
> today...
> 
> Anyway, dropped this.

Also, this is a bugfix (certain platforms are currently crashing on boot) and
once review is settled should go into 4.4.

-Scott

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Scott Wood]

On Tue, 2015-11-24 at 11:56 -0500, Timur Tabi wrote:
> On Tue, Nov 24, 2015 at 11:54 AM, Timur Tabi <timur@tabi.org> wrote:
> > Well, if you're concern is that there's no platform code, then there
> > should be a check that says, "see if there's any platform code", not
> > "let's check this obscure function and abort without explanation if
> > it's not initialized."
> > 
> > Alternatively, why can't you just do this, in update_lcdc():
> 
> [Stupid gmail sent my message before I was done typing]
> 
> if (diu_ops.set_pixel_clock)
>    diu_ops.set_pixel_clock(var->pixclock);

Because it's more obviously correct to abort the probe than to continue with
some operations nooped.  This is meant to be a quick and obvious fix to the
crashing bug.  There's another patch pending to reorganize the init of this
driver.

FWIW, I don't like the fact that this driver requires "platform code" at all. 
 Why isn't it self-contained, with knowledge of the relevant boards?

-Scott

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Timur Tabi]

On Tue, Nov 24, 2015 at 11:55 AM, Scott Wood <scottwood@freescale.com> wrote:

>> Well, if you're concern is that there's no platform code, then there
>> should be a check that says, "see if there's any platform code", not
>> "let's check this obscure function and abort without explanation if
>> it's not initialized."
>
> Do you have a *specific* better way to "see if there's any platform code"?

Well, for one thing, the check should be done in the _init function,
not the _probe.  Secondly, it should be documented as such, e.g. "/*
Check to see that we have platform code that initializes diu_ops.  If
not, then abort. */".  Third, you should probably add a boolean field
to platform_diu_data_ops that gets set to True if/when the platform
code initializes the rest of the structure.

Of course, an even better solution would be to get rid of the global
structure altogether and come up with something more robust, but I
understand that that's overkill.

RE: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Wang Dongsheng]

PiBPbiBUdWUsIE5vdiAyNCwgMjAxNSBhdCAxMTo1NCBBTSwgVGltdXIgVGFiaSA8dGltdXJAdGFi
aS5vcmc+IHdyb3RlOg0KPiA+IFdlbGwsIGlmIHlvdSdyZSBjb25jZXJuIGlzIHRoYXQgdGhlcmUn
cyBubyBwbGF0Zm9ybSBjb2RlLCB0aGVuIHRoZXJlDQo+ID4gc2hvdWxkIGJlIGEgY2hlY2sgdGhh
dCBzYXlzLCAic2VlIGlmIHRoZXJlJ3MgYW55IHBsYXRmb3JtIGNvZGUiLCBub3QNCj4gPiAibGV0
J3MgY2hlY2sgdGhpcyBvYnNjdXJlIGZ1bmN0aW9uIGFuZCBhYm9ydCB3aXRob3V0IGV4cGxhbmF0
aW9uIGlmDQo+ID4gaXQncyBub3QgaW5pdGlhbGl6ZWQuIg0KPiA+DQo+ID4gQWx0ZXJuYXRpdmVs
eSwgd2h5IGNhbid0IHlvdSBqdXN0IGRvIHRoaXMsIGluIHVwZGF0ZV9sY2RjKCk6DQo+IA0KPiBb
U3R1cGlkIGdtYWlsIHNlbnQgbXkgbWVzc2FnZSBiZWZvcmUgSSB3YXMgZG9uZSB0eXBpbmddDQo+
IA0KPiBpZiAoZGl1X29wcy5zZXRfcGl4ZWxfY2xvY2spDQo+ICAgIGRpdV9vcHMuc2V0X3BpeGVs
X2Nsb2NrKHZhci0+cGl4Y2xvY2spOw0KDQpIZXJlIGFyZSBub3QgZnJpZW5kbHkgZm9yIGtlcm5l
bCwgYmVjYXVzZSBpZiB3ZSBsb3N0IHRoZSBjbG9jayB3ZSBjYW5ub3QNCnRvIGRpc3BsYXkuIFdl
IGRvIHNvIG11Y2ggdGhpbmdzIGluIGtlcm5lbCBidXQgd2Ugc3RpbGwgY2Fubm90IHRvIGRpc3Bs
YXkgdGhhdA0KaXMgYmVmb3JlIHdlIGNhbiBrbm93IGl0Li4uU28gc3RpbGwgdGhpbmsgd2UgbmVl
ZCB0byBjaGVjayB0aGUgaG9vayBpbiBpbml0aWFsaXphdGlvbg0KZmxvdy4NCg0KUmVnYXJkcywN
Ci1Eb25nc2hlbmcNCg=

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Timur Tabi]

On Tue, Nov 24, 2015 at 12:00 PM, Scott Wood <scottwood@freescale.com> wrote:
>
> FWIW, I don't like the fact that this driver requires "platform code" at all.
>  Why isn't it self-contained, with knowledge of the relevant boards?

Yeah, I was never crazy about that either.  To make the change you
want, we would need Dongsheng's other patch that moves all hardware
init into the _probe function first.  And then we would need to get
rid of the #ifdefs for 5121.  I'm all for that, but at the time this
code was written, no one expected Freescale to continue using the DIU
after the P1022.

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Scott Wood]

On Tue, 2015-11-24 at 12:02 -0500, Timur Tabi wrote:
> On Tue, Nov 24, 2015 at 11:55 AM, Scott Wood <scottwood@freescale.com>
> wrote:
> 
> > > Well, if you're concern is that there's no platform code, then there
> > > should be a check that says, "see if there's any platform code", not
> > > "let's check this obscure function and abort without explanation if
> > > it's not initialized."
> > 
> > Do you have a *specific* better way to "see if there's any platform code"?
> 
> Well, for one thing, the check should be done in the _init function,
> not the _probe. 

I asked Dongsheng to put it in probe() during internal review because at the
time he was printing an error, and I didn't want the error to be printed if
the device wasn't present.  Again, there's another non-bugfix patch pending
that moves all the rest into probe() where it belongs.

>  Secondly, it should be documented as such, e.g. "/*
> Check to see that we have platform code that initializes diu_ops.  If
> not, then abort. */".

OK.

> Third, you should probably add a boolean field
> to platform_diu_data_ops that gets set to True if/when the platform
> code initializes the rest of the structure.

Why do you want to complicate a simple bugfix with a requirement to modify all
platforms that use the driver, introducing a possible regression if one is
missed?

-Scott

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Timur Tabi]

On Tue, Nov 24, 2015 at 12:02 PM, Wang Dongsheng
<Dongsheng.Wang@freescale.com> wrote:
>
> Here are not friendly for kernel, because if we lost the clock we cannot
> to display. We do so much things in kernel but we still cannot to display that
> is before we can know it...So still think we need to check the hook in initialization
> flow.

Ok, I agree with that.  But the check has to be more obvious, and I
think it should be done in the _init code.

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Scott Wood]

On Tue, 2015-11-24 at 10:59 -0600, Scott Wood wrote:
> On Tue, 2015-11-24 at 18:15 +0200, Tomi Valkeinen wrote:
> > 
> > On 24/11/15 18:04, Timur Tabi wrote:
> > > On Tue, Nov 24, 2015 at 5:46 AM, Tomi Valkeinen <tomi.valkeinen@ti.com>
> > > wrote:
> > > > On 24/11/15 08:27, Dongsheng Wang wrote:
> > > > > @@ -1697,6 +1700,9 @@ static int fsl_diu_probe(struct
> > > > > platform_device
> > > > > *pdev)
> > > > >       unsigned int i;
> > > > >       int ret;
> > > > > 
> > > > > +     if (!diu_ops.set_pixel_clock)
> > > > > +             return -ENODEV;
> > > > > +
> > > > >       data = dmam_alloc_coherent(&pdev->dev, sizeof(struct
> > > > > fsl_diu_data),
> > > > >                                  &dma_addr, GFP_DMA | __GFP_ZERO);
> > > > >       if (!data)
> > > > > 
> > > > 
> > > > Thanks, queued for 4.5.
> > > 
> > > Could you please wait for me to review the patch first?  I am the
> > > maintainer for the driver, and I see a problem with it.
> > 
> > Sorry, I was too hasty (and tired). I thought I was looking at a patch
> > that's been on the list for a while, but apparently it was only posted
> > today...
> > 
> > Anyway, dropped this.
> 
> Also, this is a bugfix (certain platforms are currently crashing on boot)
> and
> once review is settled should go into 4.4.

...and stable, since the defconfig changes that enabled this driver on the
problematic platforms went into 4.3.

-Scott

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Timur Tabi]

On Tue, Nov 24, 2015 at 12:05 PM, Scott Wood <scottwood@freescale.com> wrote:

> I asked Dongsheng to put it in probe() during internal review because at the
> time he was printing an error, and I didn't want the error to be printed if
> the device wasn't present.  Again, there's another non-bugfix patch pending
> that moves all the rest into probe() where it belongs.

I think it should be in _init, and not display an error.

>> Third, you should probably add a boolean field
>> to platform_diu_data_ops that gets set to True if/when the platform
>> code initializes the rest of the structure.
>
> Why do you want to complicate a simple bugfix with a requirement to modify all
> platforms that use the driver, introducing a possible regression if one is
> missed?

Fair enough, but I think it should at least be documented by saying
something about set_pixel_clock must be defined, so if it isn't, then
that means the platform code does not support DIU at all, so just
abort.

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Scott Wood]

On Tue, 2015-11-24 at 12:16 -0500, Timur Tabi wrote:
> On Tue, Nov 24, 2015 at 12:05 PM, Scott Wood <scottwood@freescale.com>
> wrote:
> 
> > I asked Dongsheng to put it in probe() during internal review because at
> > the
> > time he was printing an error, and I didn't want the error to be printed
> > if
> > the device wasn't present.  Again, there's another non-bugfix patch
> > pending
> > that moves all the rest into probe() where it belongs.
> 
> I think it should be in _init, and not display an error.

As long as it doesn't display anything I don't care much either way.

> > > Third, you should probably add a boolean field
> > > to platform_diu_data_ops that gets set to True if/when the platform
> > > code initializes the rest of the structure.
> > 
> > Why do you want to complicate a simple bugfix with a requirement to modify
> > all
> > platforms that use the driver, introducing a possible regression if one is
> > missed?
> 
> Fair enough, but I think it should at least be documented by saying
> something about set_pixel_clock must be defined, so if it isn't, then
> that means the platform code does not support DIU at all, so just
> abort.

Sure.

-Scott

RE: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Wang Dongsheng]

PiBPbiBUdWUsIDIwMTUtMTEtMjQgYXQgMTI6MTYgLTA1MDAsIFRpbXVyIFRhYmkgd3JvdGU6DQo+
ID4gT24gVHVlLCBOb3YgMjQsIDIwMTUgYXQgMTI6MDUgUE0sIFNjb3R0IFdvb2QgPHNjb3R0d29v
ZEBmcmVlc2NhbGUuY29tPg0KPiA+IHdyb3RlOg0KPiA+DQo+ID4gPiBJIGFza2VkIERvbmdzaGVu
ZyB0byBwdXQgaXQgaW4gcHJvYmUoKSBkdXJpbmcgaW50ZXJuYWwgcmV2aWV3DQo+ID4gPiBiZWNh
dXNlIGF0IHRoZSB0aW1lIGhlIHdhcyBwcmludGluZyBhbiBlcnJvciwgYW5kIEkgZGlkbid0IHdh
bnQgdGhlDQo+ID4gPiBlcnJvciB0byBiZSBwcmludGVkIGlmIHRoZSBkZXZpY2Ugd2Fzbid0IHBy
ZXNlbnQuICBBZ2FpbiwgdGhlcmUncw0KPiA+ID4gYW5vdGhlciBub24tYnVnZml4IHBhdGNoIHBl
bmRpbmcgdGhhdCBtb3ZlcyBhbGwgdGhlIHJlc3QgaW50bw0KPiA+ID4gcHJvYmUoKSB3aGVyZSBp
dCBiZWxvbmdzLg0KPiA+DQo+ID4gSSB0aGluayBpdCBzaG91bGQgYmUgaW4gX2luaXQsIGFuZCBu
b3QgZGlzcGxheSBhbiBlcnJvci4NCj4gDQo+IEFzIGxvbmcgYXMgaXQgZG9lc24ndCBkaXNwbGF5
IGFueXRoaW5nIEkgZG9uJ3QgY2FyZSBtdWNoIGVpdGhlciB3YXkuDQo+IA0KDQpTbyB3ZSBtb3Zl
IHRoZSBjb2RlIHRvIF9pbml0PyBJZiB3ZSBhcmUgY29uc2lzdGVudCBJIHdpbGwgbW92ZSB0byBf
aW5pdA0KYW5kIHNlbmQgdjIgcGF0Y2guDQoNClJlZ2FyZHMsDQotRG9uZ3NoZW5nDQo

Re: [PATCH] video: fbdev: fsl: fix kernel crash when diu_ops is not implemented

[Timur Tabi]

Wang Dongsheng wrote:
> So we move the code to _init? If we are consistent I will move to _init
> and send v2 patch.

Yes, please.