Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Abhishek Kumar <kuabhs@chromium.org>
To:     kuabhs@chromium.org
Cc:     ath10k@lists.infradead.org, briannorris@chromium.org,
        cjhuang@codeaurora.org, dianders@chromium.org,
        johannes@sipsolutions.net, linux-wireless@vger.kernel.org
Subject: Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.
Date:   Sat, 31 Oct 2020 02:46:31 +0000
Message-Id: <20201031024631.1528113-1-kuabhs@chromium.org>
In-Reply-To: <20201030205639.1452712-1-kuabhs@chromium.org>
References: <20201030205639.1452712-1-kuabhs@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=y
Content-Transfer-Encoding: 8bit
Precedence: bulk

From: kuabhs@chromium.org

There are few more additional comments here.
> + * @NL80211_CMD_SET_SAR_SPECS: SAR power limitation configuration is
> + *     passed using %NL80211_ATTR_SAR_SPEC.
> + *

This command requires NL80211_ATTR_IFINDEX, probably should be better to add
this in the comment ?

> +static int
> +nl80211_put_sar_specs(struct cfg80211_registered_device *rdev,
> +                     struct sk_buff *msg)
> +{
> +       struct nlattr *sar_capa, *specs, *sub_freq_range;
> +       u8  num_freq_ranges;
> +       int i;
> +
> +       if (!rdev->wiphy.sar_capa)
> +               return 0;
> +
> +       num_freq_ranges = rdev->wiphy.sar_capa->num_freq_ranges;
> +
> +       sar_capa = nla_nest_start(msg, NL80211_ATTR_SAR_SPEC);
> +       if (!sar_capa)
> +               return -ENOSPC;

I see some places nla_nest_start_noflag being used and here nla_nest_start.
Whats the specific reason to do that ? In the newer Kernel versions, I believe
nla_nest_start is preferred.

> +               power = nla_get_u8(spec[NL80211_SAR_ATTR_SPECS_POWER]);
> +               sar_spec->sub_specs[specs].power = power;
> +
> +               /* if NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX isn't present,
> +                * then the power applies to all bands. But it's only valid
> +                * for the first entry.
> +                */
> +               if (!spec[NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX]) {
> +                       if (specs) {
> +                               err = -EINVAL;
> +                               goto error;
> +                       } else {
> +                               sar_spec->sub_specs[specs].freq_range_index =
> +                                       NL80211_SAR_ALL_FREQ_RNAGES;
> +                               specs++;
> +                               break;
> +                       }
> +               }

Here I see you are assigning same power to all freq band if for the first band
the freq index is not provided. Is there any specific reason to only check the first
here ? Probably this logic should move into specific drivers. Thoughts ?

-Abhishek