Re: [patch] [media] radio-si476x: check different function pointers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Apr 11, 2013 at 11:24 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On Wed April 10 2013 13:40:51 Dan Carpenter wrote:
>> This is a static checker where it complains if we check for one function
>> pointer and then call a different function on the next line.
>>
>> In most cases, the code does the same thing before and after this patch.
>> For example, when ->phase_diversity is non-NULL then ->phase_div_status
>> is also non-NULL.
>>
>> The one place where that's not true is when we check ->rds_blckcnt
>> instead of ->rsq_status.  In those cases, we would want to call
>> ->rsq_status but we instead return -ENOENT.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>> ---
>> Please review this carefully.  I don't have the hardware to test it.
>
> Andrey, can you review this? I think the first two chunks are correct, but
> the last two chunks are probably not what you want. In the case of an AM
> receiver there is no RDS data, so an error is probably correct.
>

Sorry, I suck at gmail-ing and my response to this letter was bounced
for having HTML in it and I guess you didn't receive it either, I'll
just copy it below:

====================================

On Wed, Apr 10, 2013 at 4:40 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> This is a static checker where it complains if we check for one function
> pointer and then call a different function on the next line.
>
> In most cases, the code does the same thing before and after this patch.
> For example, when ->phase_diversity is non-NULL then ->phase_div_status
> is also non-NULL.
>
> The one place where that's not true is when we check ->rds_blckcnt
> instead of ->rsq_status.  In those cases, we would want to call
> ->rsq_status but we instead return -ENOENT.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> ---
> Please review this carefully.  I don't have the hardware to test it.
>
> diff --git a/drivers/media/radio/radio-si476x.c b/drivers/media/radio/radio-si476x.c
> index 9430c6a..817fc0c 100644
> --- a/drivers/media/radio/radio-si476x.c
> +++ b/drivers/media/radio/radio-si476x.c
> @@ -854,7 +854,7 @@ static int si476x_radio_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
>         switch (ctrl->id) {
>         case V4L2_CID_SI476X_INTERCHIP_LINK:
>                 if (si476x_core_has_diversity(radio->core)) {
> -                       if (radio->ops->phase_diversity) {
> +                       if (radio->ops->phase_div_status) {
>                                 retval = radio->ops->phase_div_status(radio->core);
>                                 if (retval < 0)
>                                         break;

I think I would prefer to use "si476x_core_is_in_am_receiver_mode" in
the case above and then have additional
BUG_ON(radio->ops->phase_div_status) for cases where it should not be
NULL(tuner in FM mode, diversity feature present) also I probably
should return -EINVAL in all the cases for this control.



> @@ -1285,7 +1285,7 @@ static ssize_t si476x_radio_read_agc_blob(struct file *file,
>         struct si476x_agc_status_report report;
>
>         si476x_core_lock(radio->core);
> -       if (radio->ops->rds_blckcnt)
> +       if (radio->ops->agc_status)
>                 err = radio->ops->agc_status(radio->core, &report);
>         else
>                 err = -ENOENT;
> @@ -1320,7 +1320,7 @@ static ssize_t si476x_radio_read_rsq_blob(struct file *file,
>         };
>
>         si476x_core_lock(radio->core);
> -       if (radio->ops->rds_blckcnt)
> +       if (radio->ops->rsq_status)
>                 err = radio->ops->rsq_status(radio->core, &args, &report);
>         else
>                 err = -ENOENT;
> @@ -1355,7 +1355,7 @@ static ssize_t si476x_radio_read_rsq_primary_blob(struct file *file,
>         };
>
>         si476x_core_lock(radio->core);
> -       if (radio->ops->rds_blckcnt)
> +       if (radio->ops->rsq_status)
>                 err = radio->ops->rsq_status(radio->core, &args, &report);
>         else
>                 err = -ENOENT;

This all looks like a dumb copy-paste screw up on my part. I think I
copied the body of "si476x_radio_read_rds_blckcnt_blob" and used it to
implement all the other functions and I guess forgot to change this
bit of the code. Thank you for catching this, mistakes like this are
just embarrassing.

I'll make a patch based on this one and send it alongside the patches
for MFD subsystem
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux