Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <b4a1d4e1-a7b9-00f9-75d2-56003138883f@roeck-us.net>
Date:   Sun, 27 Feb 2022 07:01:32 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Content-Language: en-US
To:     Zev Weiss <zev@bewilderbeest.net>, linux-hwmon@vger.kernel.org,
        Jean Delvare <jdelvare@suse.com>
Cc:     Renze Nicolai <renze@rnplus.nl>, openbmc@lists.ozlabs.org,
        linux-kernel@vger.kernel.org
References: <20220226133047.6226-1-zev@bewilderbeest.net>
 <20220226133047.6226-2-zev@bewilderbeest.net>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH 1/5] hwmon: (nct6775) Rearrange attr-group initialization
In-Reply-To: <20220226133047.6226-2-zev@bewilderbeest.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/26/22 05:30, Zev Weiss wrote:
> We now track the number of attribute groups in nct6775_data, as a
> measure to simplify handling differences in the set of enabled
> attribute groups between nct6775 drivers (platform & i2c).  As a side
> effect, we also reduce the amount of IS_ERR()/PTR_ERR() boilerplate a
> bit.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>   drivers/hwmon/nct6775.c | 84 ++++++++++++++++++++---------------------
>   1 file changed, 42 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
> index 2b91f7e05126..139b2fe5ca4d 100644
> --- a/drivers/hwmon/nct6775.c
> +++ b/drivers/hwmon/nct6775.c
> @@ -1198,6 +1198,7 @@ struct nct6775_data {
>   	const char *name;
>   
>   	const struct attribute_group *groups[7];
> +	u8 num_groups;
>   
>   	u16 reg_temp[5][NUM_TEMP]; /* 0=temp, 1=temp_over, 2=temp_hyst,
>   				    * 3=temp_crit, 4=temp_lcrit
> @@ -1405,10 +1406,18 @@ struct sensor_template_group {
>   	int base;
>   };
>   
> -static struct attribute_group *
> -nct6775_create_attr_group(struct device *dev,
> -			  const struct sensor_template_group *tg,
> -			  int repeat)
> +static int nct6775_add_attr_group(struct nct6775_data *data, const struct attribute_group *group)
> +{
> +	/* Need to leave a NULL terminator at the end of data->groups */
> +	if (WARN_ON(data->num_groups == ARRAY_SIZE(data->groups) - 1))
> +		return -ENOSPC;
> +

At work we are struggling with a whopping 500,000+ (!) WARN backtraces
_each day_. I would be happy if you send me patches removing some of
those, but I am not inclined to accept patches adding them. If people
don't notice that the driver doesn't load, they won't notice the warning
either, and it will just add to all the other warning backtrace noise.

Guenter

> +	data->groups[data->num_groups++] = group;
> +	return 0;
> +}
> +
> +static int nct6775_add_template_attr_group(struct device *dev, struct nct6775_data *data,
> +					   const struct sensor_template_group *tg, int repeat)
>   {
>   	struct attribute_group *group;
>   	struct sensor_device_attr_u *su;
> @@ -1419,28 +1428,28 @@ nct6775_create_attr_group(struct device *dev,
>   	int i, count;
>   
>   	if (repeat <= 0)
> -		return ERR_PTR(-EINVAL);
> +		return -EINVAL;
>   
>   	t = tg->templates;
>   	for (count = 0; *t; t++, count++)
>   		;
>   
>   	if (count == 0)
> -		return ERR_PTR(-EINVAL);
> +		return -EINVAL;
>   
>   	group = devm_kzalloc(dev, sizeof(*group), GFP_KERNEL);
>   	if (group == NULL)
> -		return ERR_PTR(-ENOMEM);
> +		return -ENOMEM;
>   
>   	attrs = devm_kcalloc(dev, repeat * count + 1, sizeof(*attrs),
>   			     GFP_KERNEL);
>   	if (attrs == NULL)
> -		return ERR_PTR(-ENOMEM);
> +		return -ENOMEM;
>   
>   	su = devm_kzalloc(dev, array3_size(repeat, count, sizeof(*su)),
>   			       GFP_KERNEL);
>   	if (su == NULL)
> -		return ERR_PTR(-ENOMEM);
> +		return -ENOMEM;
>   
>   	group->attrs = attrs;
>   	group->is_visible = tg->is_visible;
> @@ -1478,7 +1487,7 @@ nct6775_create_attr_group(struct device *dev,
>   		}
>   	}
>   
> -	return group;
> +	return nct6775_add_attr_group(data, group);
>   }
>   
>   static bool is_word_sized(struct nct6775_data *data, u16 reg)
> @@ -4020,10 +4029,8 @@ static int nct6775_probe(struct platform_device *pdev)
>   	const u16 *reg_temp_crit_l = NULL, *reg_temp_crit_h = NULL;
>   	int num_reg_temp, num_reg_temp_mon, num_reg_tsi_temp;
>   	u8 cr2a;
> -	struct attribute_group *group;
>   	struct device *hwmon_dev;
>   	struct sensor_template_group tsi_temp_tg;
> -	int num_attr_groups = 0;
>   
>   	if (sio_data->access == access_direct) {
>   		res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> @@ -4844,46 +4851,39 @@ static int nct6775_probe(struct platform_device *pdev)
>   	nct6775_init_fan_common(dev, data);
>   
>   	/* Register sysfs hooks */
> -	group = nct6775_create_attr_group(dev, &nct6775_pwm_template_group,
> -					  data->pwm_num);
> -	if (IS_ERR(group))
> -		return PTR_ERR(group);
> -
> -	data->groups[num_attr_groups++] = group;
> -
> -	group = nct6775_create_attr_group(dev, &nct6775_in_template_group,
> -					  fls(data->have_in));
> -	if (IS_ERR(group))
> -		return PTR_ERR(group);
> -
> -	data->groups[num_attr_groups++] = group;
> -
> -	group = nct6775_create_attr_group(dev, &nct6775_fan_template_group,
> -					  fls(data->has_fan));
> -	if (IS_ERR(group))
> -		return PTR_ERR(group);
> +	err = nct6775_add_template_attr_group(dev, data, &nct6775_pwm_template_group,
> +					      data->pwm_num);
> +	if (err)
> +		return err;
>   
> -	data->groups[num_attr_groups++] = group;
> +	err = nct6775_add_template_attr_group(dev, data, &nct6775_in_template_group,
> +					      fls(data->have_in));
> +	if (err)
> +		return err;
>   
> -	group = nct6775_create_attr_group(dev, &nct6775_temp_template_group,
> -					  fls(data->have_temp));
> -	if (IS_ERR(group))
> -		return PTR_ERR(group);
> +	err = nct6775_add_template_attr_group(dev, data, &nct6775_fan_template_group,
> +					      fls(data->has_fan));
> +	if (err)
> +		return err;
>   
> -	data->groups[num_attr_groups++] = group;
> +	err = nct6775_add_template_attr_group(dev, data, &nct6775_temp_template_group,
> +					      fls(data->have_temp));
> +	if (err)
> +		return err;
>   
>   	if (data->have_tsi_temp) {
>   		tsi_temp_tg.templates = nct6775_tsi_temp_template;
>   		tsi_temp_tg.is_visible = nct6775_tsi_temp_is_visible;
>   		tsi_temp_tg.base = fls(data->have_temp) + 1;
> -		group = nct6775_create_attr_group(dev, &tsi_temp_tg, fls(data->have_tsi_temp));
> -		if (IS_ERR(group))
> -			return PTR_ERR(group);
> -
> -		data->groups[num_attr_groups++] = group;
> +		err = nct6775_add_template_attr_group(dev, data, &tsi_temp_tg,
> +						      fls(data->have_tsi_temp));
> +		if (err)
> +			return err;
>   	}
>   
> -	data->groups[num_attr_groups++] = &nct6775_group_other;
> +	err = nct6775_add_attr_group(data, &nct6775_group_other);
> +	if (err)
> +		return err;
>   
>   	hwmon_dev = devm_hwmon_device_register_with_groups(dev, data->name,
>   							   data, data->groups);