[fix quoting, adding broonie and linux-spi to addressees]
On Mon, Oct 05, 2015 at 07:43:37AM +0200, Sean Nyekjær wrote:
> On 2015-10-04 22:23, Uwe Kleine-König wrote:
> >On Mon, Aug 31, 2015 at 12:01:27PM +0200, Sean Nyekjaer wrote:
> >>If a wrong compatible flag in specified, the of_match_device
> >>returning null.
> >>Implemented check and if NULL then returning -ENODEV.
> >>
> >>Signed-off-by: Sean Nyekjaer <[email protected]>
> >>---
> >> drivers/tty/serial/sc16is7xx.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >>diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> >>index 72ffd0d..2fce9fb 100644
> >>--- a/drivers/tty/serial/sc16is7xx.c
> >>+++ b/drivers/tty/serial/sc16is7xx.c
> >>@@ -1320,6 +1320,8 @@ static int sc16is7xx_spi_probe(struct spi_device *spi)
> >> if (spi->dev.of_node) {
> >> const struct of_device_id *of_id =
> >> of_match_device(sc16is7xx_dt_ids, &spi->dev);
> >>+ if (!of_id)
> >>+ return -ENODEV;
> >
> >Did you see a problem here? I wonder if this can ever trigger. In the
>
> Yes i did :-)
>
> >commit log you wrote "If a wrong compatible flag in specified, ...", but
> >then the probe function isn't called at all, is it?
> >
> >> devtype = (struct sc16is7xx_devtype *)of_id->data;
> >> } else {
>
> Here you can see code snippet of the dtb that triggered the NULL
> pointer dereference:
> sc16is750@1 {
> compatible = "sc16is750";
> spi-max-frequency = <2000000>;
> reg = <1>;
> clocks = <&clk14m>;
> interrupt-parent = <&gpio7>;
> interrupts = <5 2>;
> }
>
> And what my dtb should have looked like:
> sc16is750@1 {
> compatible = "nxp,sc16is750";
> spi-max-frequency = <2000000>;
> reg = <1>;
> clocks = <&clk14m>;
> interrupt-parent = <&gpio7>;
> interrupts = <5 2>;
> }
The relevant driver details are:
static const struct of_device_id __maybe_unused sc16is7xx_dt_ids[] = {
...
{ .compatible = "nxp,sc16is750", .data = &sc16is750_devtype, },
...
{ }
};
...
static const struct spi_device_id sc16is7xx_spi_id_table[] = {
...
{ "sc16is750", (kernel_ulong_t)&sc16is750_devtype, },
...
{ }
};
So in the broken case the device in question was bound to the
sc16is7xx driver because the compatible name was matched by the
id_table and not the of_match_table. My first thought was that in this
case the spi_device's dev->of_node should not be set, but after having
thought a bit more I think this is right.
But the .probe routine could be a tad more clever here:
devtype = of_device_get_match_data(&spi->dev);
if (!devtype) {
const struct spi_device_id *id_entry = spi_get_device_id(spi);
/* Do I need to check id_entry being != NULL here? */
devtype = (struct sc16is7xx_devtype *)id_entry->driver_data;
flags = IRQF_TRIGGER_FALLING;
}
Apart from that using "flags = IRQF_TRIGGER_FALLING" iff the device was
probed via spi_id_table is strange.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
