| Message ID | 1448346450-47403-1-git-send-email-dongsheng.wang@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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. -- 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
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. -- 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
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 -- 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
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
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 -- 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
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 -- 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
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); -- 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
Hi Timur, Thanks for your review. > 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. Another patch video: fbdev: fsl: Split DIU initialization entry:[https://patchwork.kernel.org/patch/7381351/] module_init(fsl_diu_init) will be replace, and all of the initialization will be completed in the probe include this check. So just do a quick fix for this boot kernel crash issue. Regards, -Dongsheng
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 -- 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
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 -- 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
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. -- 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
> 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); 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. Regards, -Dongsheng
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. -- 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
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 -- 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
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. -- 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
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 -- 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
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. -- 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
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 -- 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
> 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. > So we move the code to _init? If we are consistent I will move to _init and send v2 patch. Regards, -Dongsheng
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. -- 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
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)