pata_platform: Fix NULL pointer dereference

PATCH In review Magnus Damm 3 participants 7 messages Jul 17, 2007 – Jul 18, 2007 libata

lore

Share & tools

lore

Magnus Damm

author

2007-07-17 08:42 UTC #0 lore raw

PATCH 1/1 pata_platform: Fix NULL pointer dereference

pata_platform: Fix NULL pointer dereference

The platform-specific structures may leave pdev->dev.platform_data as NULL.

Signed-off-by Magnus Damm <[email protected]>

---

Tested on a Solution Engline 7722
Applies to git linux-2.6 a5fcaa210626a79465321e344c91a6a7dc3881fa

 drivers/ata/pata_platform.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/ata/pata_platform.c +3 -2

@@ -213,8 +213,9 @@ static int __devinit pata_platform_probe
 	pata_platform_setup_port(&ap->ioaddr, pp_info);
 
 	/* activate */
-	return ata_host_activate(host, platform_get_irq(pdev, 0), ata_interrupt,
-				 pp_info->irq_flags, &pata_platform_sht);
+	return ata_host_activate(host, platform_get_irq(pdev, 0), 
+				 ata_interrupt, pp_info ? pp_info->irq_flags
+				 : 0, &pata_platform_sht);
 }
 
 /**

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Paul Mundt

2007-07-17 09:03 UTC #1 lore raw

Quoted 5 lines · Magnus Damm

pata_platform: Fix NULL pointer dereference
The platform-specific structures may leave pdev->dev.platform_data as NULL.
Signed-off-by Magnus Damm <[email protected]>

Looks like the change that broke this came in the libata merge, more specifically this change:

commit 5f45bc50976ee1f408f7171af155aec646655a37

Author: Sonic Zhang <[email protected]>
Date:   Fri Jun 15 17:45:49 2007 +0800

Add irq_flags to struct pata_platform_info

On some embedded platforms, such as blackfin, the gpio interrupt for IDE interface is designed to be triggered with high voltage. The gpio port should be configured properly by set_irq_type() when register the irq. This patch enable the generic pata platform driver to accept platform irq flags data.

Signed-off-by Sonic Zhang <[email protected]>

Signed-off-by Jeff Garzik <[email protected]>

It would be great if libata people could actually be bothered to CC driver authors on driver changes so that way these things don't get completely broken in mainline when simple testing on the platforms that actually _rely_ on this driver would have shown that this was broken.

I don't intend to poll the various libata branches on the off chance I'll find something that's hosed my platforms, and I don't think it's an unreasonable expectation to at least have to see the patch and sign off on it before it gets merged. Every other subsystem works this way, I wonder why libata feels that it's special.

This is also not the first time changes to pata_platform have caused fallout, without finding out about it until after the changes were already merged.

Jeff Garzik

2007-07-17 09:33 UTC #2 lore raw

Quoted 30 lines · Paul Mundt

Quoted 5 lines · Magnus Damm
pata_platform: Fix NULL pointer dereference
The platform-specific structures may leave pdev->dev.platform_data as NULL.
Signed-off-by Magnus Damm <[email protected]>
Looks like the change that broke this came in the libata merge, more specifically this change:
commit 5f45bc50976ee1f408f7171af155aec646655a37
Author: Sonic Zhang <[email protected]>
Date:   Fri Jun 15 17:45:49 2007 +0800
Add irq_flags to struct pata_platform_info
On some embedded platforms, such as blackfin, the gpio interrupt for IDE interface is designed to be triggered with high voltage. The gpio port should be configured properly by set_irq_type() when register the irq. This patch enable the generic pata platform driver to accept platform irq flags data.
Signed-off-by Sonic Zhang <[email protected]>
Signed-off-by Jeff Garzik <[email protected]>
It would be great if libata people could actually be bothered to CC driver authors on driver changes so that way these things don't get completely broken in mainline when simple testing on the platforms that actually _rely_ on this driver would have shown that this was broken.

Quoted 8 lines

I don't intend to poll the various libata branches on the off chance I'll find something that's hosed my platforms, and I don't think it's an unreasonable expectation to at least have to see the patch and sign off on it before it gets merged. Every other subsystem works this way, I wonder why libata feels that it's special.
This is also not the first time changes to pata_platform have caused fallout, without finding out about it until after the changes were already merged.

The patch lived for weeks in -mm along with tons of other git trees Andrew pulls. If you want a single point to watch for upcoming stuff, test the -mm trees. It's a key part of the development process: I merge a patch, -mm tree pulls my tree and others, people test and complain and give feedback, ...

With embedded stuff its tough to judge whether people are knowledgeable about the impact of the change, and sometimes I judge wrongly. My apologies.

What is your suggestion for moving linux-2.6.git forward?

Jeff

Paul Mundt

2007-07-17 09:47 UTC #3 lore raw

Quoted 7 lines · Jeff Garzik

Quoted 5 lines · Paul Mundt
It would be great if libata people could actually be bothered to CC driver authors on driver changes so that way these things don't get completely broken in mainline when simple testing on the platforms that actually _rely_ on this driver would have shown that this was broken.

Quoted 6 lines

The patch lived for weeks in -mm along with tons of other git trees Andrew pulls. If you want a single point to watch for upcoming stuff, test the -mm trees. It's a key part of the development process: I merge a patch, -mm tree pulls my tree and others, people test and complain and give feedback, ...

A key part of the development process is making sure that driver authors are aware of the changes being made to their drivers, so they're able to ACK/NACK or at least point out something that's obviously wrong. I test -mm when time permits, and this happened to slip through. If I had actually been CC'ed on it, it would not have.

Your entire process is fundamentally flawed if you're merging the patch first and expecting people to only find out if it's broken after the fact. In the best case it leaves -mm broken for a single release, and in the other case, it happens to make its way to mainline before anyone notices.

Quoted 4 lines

With embedded stuff its tough to judge whether people are knowledgeable about the impact of the change, and sometimes I judge wrongly. My apologies.

Another reason to make sure the authors are CC'ed. I really don't see why this is so difficult for you, _everyone_ else manages to do this properly, and shis sort of case illustrates _exactly_ why.

Quoted 2 lines

What is your suggestion for moving linux-2.6.git forward?

The patch from Magnus is fine with me for fixing this particular problem, so at least getting that merged quickly will get the sh and ppc platforms fixed again.

To avoid having this happen again in the future, I'd appreciate being CC'ed on things that impact pata_platform pre-merge, and again, I don't see this as an extraordinarly unreasonable request. If you can't be bothered doing that, then just stop applying pata_platform patches, and I'll merge them myself.

Jeff Garzik

2007-07-17 10:24 UTC #4 lore raw

Quoted 19 lines · Paul Mundt

Quoted 6 lines · Jeff Garzik
Quoted 5 lines · Paul Mundt
It would be great if libata people could actually be bothered to CC driver authors on driver changes so that way these things don't get completely broken in mainline when simple testing on the platforms that actually _rely_ on this driver would have shown that this was broken.
Quoted 6 lines
The patch lived for weeks in -mm along with tons of other git trees Andrew pulls. If you want a single point to watch for upcoming stuff, test the -mm trees. It's a key part of the development process: I merge a patch, -mm tree pulls my tree and others, people test and complain and give feedback, ...
A key part of the development process is making sure that driver authors are aware of the changes being made to their drivers, so they're able to ACK/NACK or at least point out something that's obviously wrong. I test -mm when time permits, and this happened to slip through. If I had actually been CC'ed on it, it would not have.

Quoted 5 lines

Your entire process is fundamentally flawed if you're merging the patch first and expecting people to only find out if it's broken after the fact. In the best case it leaves -mm broken for a single release, and in the other case, it happens to make its way to mainline before anyone notices.

It's just reality that there are never enough people to verify every patch before it goes in. We just support way too much hardware for that to be remotely feasible. Sure we -try- to keep the driver maintainer CC'd, but alas we are human.

Your thinking is flawed if you think I'm going to hold up every patch until it's verified on each of 63 ATA controller drivers, when I make a core change, for example. Not scalable. We scale in another way:

We have the biggest test lab in the world -- the Internet -- and a development process staged so that testing and development occur in parallel. This here is actually an example of the process working:
despite my forgetting to CC you, the problem was caught long before it made it into any release kernel.

Jeff

Paul Mundt

2007-07-17 11:01 UTC #5 lore raw

Quoted 5 lines · Jeff Garzik

It's just reality that there are never enough people to verify every patch before it goes in. We just support way too much hardware for that to be remotely feasible. Sure we -try- to keep the driver maintainer CC'd, but alas we are human.

Try? I've never _once_ received a CC from you regarding a change to pata_platform. The only time I've been CC'ed at all is when someone submitting the patch had the common courtesy to do so.

Whereas the number of times I've had to clean up after something merged behind my back has been higher than the number of times I've been CC'ed on any of these changes.

Quoted 4 lines

Your thinking is flawed if you think I'm going to hold up every patch until it's verified on each of 63 ATA controller drivers, when I make a core change, for example. Not scalable. We scale in another way:

This has nothing to do with core changes that impact every driver, this has to do with isolated changes to a _single_ driver that you're obviously not in a position to test. That's a very different situation.

Again, if you can't be bothered to CC people on changes to their drivers that aren't libata-wide, don't apply them in the first place. I would much rather play API catch up with my driver than have to backtrack and figure out what went wrong when suddenly all of my boards stop booting.

Your "merge first and hope someone else notices and fixes it later" approach is insane. We've never done kernel development like that, libata isn't special.

Magnus Damm

author

2007-07-18 09:32 UTC #6 lore raw

In reply to Jeff Garzik

Quoted 9 lines · Jeff Garzik

Quoted 6 lines · Paul Mundt
Quoted 5 lines · Magnus Damm
pata_platform: Fix NULL pointer dereference
The platform-specific structures may leave pdev->dev.platform_data as NULL.
Signed-off-by Magnus Damm <[email protected]>
What is your suggestion for moving linux-2.6.git forward?

This fix solves the problem for me and Paul seems to be happy with it as well. Jeff, can you please pick up this patch so we can unbreak things for rc1?

Thanks!

/ magnus