linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: linux@arm.linux.org.uk (Russell King - ARM Linux)
To: linux-arm-kernel@lists.infradead.org
Subject: imx6 eSATA
Date: Sat, 18 Jan 2014 19:03:37 +0000	[thread overview]
Message-ID: <20140118190337.GK15937@n2100.arm.linux.org.uk> (raw)
In-Reply-To: <20140118184427.GJ15937@n2100.arm.linux.org.uk>

On Sat, Jan 18, 2014 at 06:44:27PM +0000, Russell King - ARM Linux wrote:
> So, I see we have AHCI support for SATA on the iMX6.  Great... but it
> doesn't work on the cubox-i, because the phy settings are wrong.

And another thing.  This is wonderful... really wonderful.

static int imx_ahci_probe(struct platform_device *pdev)
{  
        struct device *dev = &pdev->dev;
...
        ahci_pdev = platform_device_alloc("ahci", -1);
...
        ahci_dev = &ahci_pdev->dev; 
...
        ahci_dev->of_node = dev->of_node;

This is a hanging offence.

Here's a lesson in how DT matching works:

- A device gets declared into the device model.
- The device is offered to each driver in turn via the bus specific
  code to see whether the driver should be bound to the device.
- If there's an of_node present, the DT IDs for the driver are walked
  to compare the device with the driver's DT IDs.  If a match is found,
  the device is passed to the driver probe function.
- If there is no of_node present, and it's a platform device, the bare
  device name is compared the driver name(s) and if it matches the
  probe function is called.

Now, what does this mean for the above monstrosity?  If the driver
model happens to find the ahci_imx driver _before_ the ahci driver
while trying to bind the ahci_dev, it will find that the ahci_dev
has an of_node with a compatible string which matches this driver.
So, imx_ahci_probe() _can_ be called with the ahci_pdev that it
just created.

It doesn't take much to understand what the result will be of that.
It will try to create another ahci device... hopefully this time
erroring out.

This is utterly disgusting.  You must *never* *ever* assign an of_node
from one device to another of the same bus type.  If you need to pass
the of_node to another device, then it _must_ be done outside of the
child device's of_node pointer - in other words, it must be done using
platform data.

Alternatively, turn ahci into a library that both the original ahci
and ahci_imx drivers can use without jumping through these kinds of
games - or in this case, just get rid of that assignment - I can't
see anything in ahci.c which needs the of_node.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

  reply	other threads:[~2014-01-18 19:03 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-01-18 18:44 imx6 eSATA Russell King - ARM Linux
2014-01-18 19:03 ` Russell King - ARM Linux [this message]
2014-01-18 21:11   ` Russell King - ARM Linux
2014-01-19  4:27     ` Shawn Guo
2014-01-19  4:15 ` Shawn Guo
2014-01-20  3:28   ` Hong-Xing.Zhu at freescale.com
2014-08-27  9:11 ` Jean-Michel Hautbois
2014-08-27  9:36   ` Russell King - ARM Linux

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140118190337.GK15937@n2100.arm.linux.org.uk \
    --to=linux@arm.linux.org.uk \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).