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

[Jonathan Cameron <jic23@kernel.org>]

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@ti.com>
>>> ---
>>>    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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>