Re: [PATCH 2/4] iccsense: convert to linked list

[Martin Peres <martin.peres-GANU6spQydw@public.gmane.org>]

On 25/03/16 13:19, Karol Herbst wrote:
> Signed-off-by: Karol Herbst <nouveau@karolherbst.de>
> ---
>   drm/nouveau/include/nvkm/subdev/iccsense.h |  4 +---
>   drm/nouveau/nouveau_hwmon.c                |  2 +-
>   drm/nouveau/nvkm/subdev/iccsense/base.c    | 32 +++++++++++++-----------------
>   drm/nouveau/nvkm/subdev/iccsense/priv.h    |  1 +
>   4 files changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/drm/nouveau/include/nvkm/subdev/iccsense.h b/drm/nouveau/include/nvkm/subdev/iccsense.h
> index c3defcd..a4c0da0 100644
> --- a/drm/nouveau/include/nvkm/subdev/iccsense.h
> +++ b/drm/nouveau/include/nvkm/subdev/iccsense.h
> @@ -3,12 +3,10 @@
>   
>   #include <core/subdev.h>
>   
> -struct nkvm_iccsense_rail;
>   struct nvkm_iccsense {
>   	struct nvkm_subdev subdev;
> -	u8 rail_count;
>   	bool data_valid;
> -	struct nvkm_iccsense_rail *rails;
> +	struct list_head rails;
>   };
>   
>   int gf100_iccsense_new(struct nvkm_device *, int index, struct nvkm_iccsense **);
> diff --git a/drm/nouveau/nouveau_hwmon.c b/drm/nouveau/nouveau_hwmon.c
> index 67edd2f..74f237b 100644
> --- a/drm/nouveau/nouveau_hwmon.c
> +++ b/drm/nouveau/nouveau_hwmon.c
> @@ -689,7 +689,7 @@ nouveau_hwmon_init(struct drm_device *dev)
>   			goto error;
>   	}
>   
> -	if (iccsense && iccsense->data_valid && iccsense->rail_count) {
> +	if (iccsense && iccsense->data_valid && !list_empty(&iccsense->rails)) {
>   		ret = sysfs_create_group(&hwmon_dev->kobj,
>   					 &hwmon_power_attrgroup);
>   		if (ret)
> diff --git a/drm/nouveau/nvkm/subdev/iccsense/base.c b/drm/nouveau/nvkm/subdev/iccsense/base.c
> index bf1b94e..6fde68d 100644
> --- a/drm/nouveau/nvkm/subdev/iccsense/base.c
> +++ b/drm/nouveau/nvkm/subdev/iccsense/base.c
> @@ -98,25 +98,21 @@ nvkm_iccsense_ina3221_read(struct nvkm_iccsense *iccsense,
>   int
>   nvkm_iccsense_read_all(struct nvkm_iccsense *iccsense)
>   {
> -	int result = 0, i;
> +	int result = 0;
> +	struct nvkm_iccsense_rail *rail;
>   
>   	if (!iccsense)
>   		return -EINVAL;
>   
> -	if (iccsense->rail_count == 0)
> -		return -ENODEV;
> -
> -	for (i = 0; i < iccsense->rail_count; ++i) {
> +	list_for_each_entry(rail, &iccsense->rails, head) {
>   		int res;
> -		struct nvkm_iccsense_rail *rail = &iccsense->rails[i];
>   		if (!rail->read)
>   			return -ENODEV;
>   
>   		res = rail->read(iccsense, rail);
> -		if (res >= 0)
> -			result += res;
> -		else
> +		if (res < 0)
>   			return res;
> +		result += res;
>   	}
>   	return result;
>   }
> @@ -125,9 +121,10 @@ static void *
>   nvkm_iccsense_dtor(struct nvkm_subdev *subdev)
>   {
>   	struct nvkm_iccsense *iccsense = nvkm_iccsense(subdev);
> +	struct nvkm_iccsense_rail *rail, *tmp;
>   
> -	if (iccsense->rails)
> -		kfree(iccsense->rails);
> +	list_for_each_entry_safe(rail, tmp, &iccsense->rails, head)
> +		kfree(rail);
The rails list is filled with invalid pointers at this point. Please add 
list_del(rail); before kfree(rail);

It has the added benefit of adding poisonous pointers that will show any 
access after freeing.
>   
>   	return iccsense;
>   }
> @@ -145,11 +142,6 @@ nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
>   	    || !stbl.nr_entry)
>   		return 0;
>   
> -	iccsense->rails = kmalloc(sizeof(*iccsense->rails) * stbl.nr_entry,
> -	                          GFP_KERNEL);
> -	if (!iccsense->rails)
> -		return -ENOMEM;
> -
>   	iccsense->data_valid = true;
>   	for (i = 0; i < stbl.nr_entry; ++i) {
>   		struct pwr_rail_t *r = &stbl.rail[i];
> @@ -184,7 +176,10 @@ nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
>   			continue;
>   		}
>   
> -		rail = &iccsense->rails[iccsense->rail_count];
> +		rail = kmalloc(sizeof(*rail), GFP_KERNEL);
> +		if (!rail)
> +			return -ENOMEM;
> +
>   		switch (extdev.type) {
>   		case NVBIOS_EXTDEV_INA209:
>   			rail->read = nvkm_iccsense_ina209_read;
> @@ -201,7 +196,7 @@ nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
>   		rail->rail = r->rail;
>   		rail->mohm = r->resistor_mohm;
>   		rail->i2c = &i2c_bus->i2c;
> -		++iccsense->rail_count;
> +		list_add_tail(&rail->head, &iccsense->rails);
>   	}
>   	return 0;
>   }
> @@ -224,6 +219,7 @@ nvkm_iccsense_new_(struct nvkm_device *device, int index,
>   {
>   	if (!(*iccsense = kzalloc(sizeof(**iccsense), GFP_KERNEL)))
>   		return -ENOMEM;
> +	INIT_LIST_HEAD(&(*iccsense)->rails);
>   	nvkm_iccsense_ctor(device, index, *iccsense);
>   	return 0;
>   }
> diff --git a/drm/nouveau/nvkm/subdev/iccsense/priv.h b/drm/nouveau/nvkm/subdev/iccsense/priv.h
> index ed398b8..e479128 100644
> --- a/drm/nouveau/nvkm/subdev/iccsense/priv.h
> +++ b/drm/nouveau/nvkm/subdev/iccsense/priv.h
> @@ -4,6 +4,7 @@
>   #include <subdev/iccsense.h>
>   
>   struct nvkm_iccsense_rail {
> +	struct list_head head;
>   	int (*read)(struct nvkm_iccsense *, struct nvkm_iccsense_rail *);
>   	struct i2c_adapter *i2c;
>   	u8 addr;

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau