Re: [PATCH 2/2] iio: inkern: add error case in iio_channel_get()

[Jonathan Cameron]

On 14/09/12 01:59, Kim, Milo wrote:
> > On 10/09/12 09:02, Kim, Milo wrote:
> > >    The datasheet name is defined in the IIO driver.
> > >    On the other hand, the adc_channel_label is configured in
> > >    the platform machine side.
> > >    If the datasheet name is not matched with any adc_channel_label,
> > >    the iio_channel_get() should be returned as error for preventing
> > >    using invalid IIO channel data.
> > >
> > >    Moreover, this patch detects NULL pointer dereference problem at
> > early time.
> > >    In general, the IIO driver just accesses to any member of the
> > iio_chan_spec
> > >    in own xxx_read_raw() function.
> > >    If the iio_chan_spec is invalid pointer, NULL dereference problem
> > may occur
> > >    such like 'iio_chan_spec->channel' or 'iio_chan_spec->type'.
> > >    If the iio_channel_get() gets failed in the IIO consumer,
> > >    then no read_raw() operation proceeds.
>
> > I guess this is marginally cleaner, but either way it is up to the
> > calling driver to check whether the call succeeded.
>
> IMO, it would be better if iio_channel_get() gets failed
> when the consumer tries to get the channel rather than when using it.
>
> The channel spec is retrieved when the channel is requested, - iio_channel_get()
> So it's reasonable if error returns in case of invalid channel spcec.
>
> And I just found similar handling in iio_channel_get_all().
> But my patch needs releasing the memory as Lars-Peter noted.
Fine, I'm convinced.  Certainly less weird doing it your way ;)
Please do fix up the get all case as well.
> Thanks !
>
> Best Regards,
> Milo
>
> > > Signed-off-by: Milo(Woogyom) Kim <milo.kim@xxxxxx>
> > > ---
> > >    drivers/iio/inkern.c |    6 +++++-
> > >    1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > > index 1faa240..a5caf6b 100644
> > > --- a/drivers/iio/inkern.c
> > > +++ b/drivers/iio/inkern.c
> > > @@ -136,11 +136,15 @@ struct iio_channel *iio_channel_get(const char
> > *name, const char *channel_name)
> > >    	channel->indio_dev = c->indio_dev;
> > >
> > > -	if (c->map->adc_channel_label)
> > > +	if (c->map->adc_channel_label) {
> > >    		channel->channel =
> > >    			iio_chan_spec_from_name(channel->indio_dev,
> > >    						c->map->adc_channel_label);
> > >
> > > +		if (channel->channel == NULL)
> > > +			return ERR_PTR(-ENODEV);
> > > +	}
> > > +
> > >    	return channel;
> > >    }
> > >    EXPORT_SYMBOL_GPL(iio_channel_get);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/