Re: [PATCH 15/18] staging: mt7621-pinctrl: refactor rt2880_pinctrl_dt_node_to_map function

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

 



On Thu, Jun 07 2018, Sergio Paracuellos wrote:

> Using 'function' and 'groups' bindings in the device tree give the
> posibility of refactor 'rt2880_pinctrl_dt_node_to_map' and simplify
> it a lot. Make use of the 'of_property_count_strings' function to get
> number of groups for the node and iterate over the groups using
> 'of_property_for_each_string' calling 'pinctrl_utils_add_map_mux'
> function which is the same of the custom function in this driver code
> 'rt2880_pinctrl_dt_subnode_to_map' which is not needed anymore.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>

Hi Sergio,
 I've just noticed a problem with this code.  The problem existed before
 your patch - you didn't introduce it.  But maybe you are more familiar
 with the code and might like to fix it...
 

> ---
>  drivers/staging/mt7621-pinctrl/pinctrl-rt2880.c | 60 ++++++++-----------------
>  1 file changed, 18 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/staging/mt7621-pinctrl/pinctrl-rt2880.c b/drivers/staging/mt7621-pinctrl/pinctrl-rt2880.c
> index 84494a1..8291ec0 100644
> --- a/drivers/staging/mt7621-pinctrl/pinctrl-rt2880.c
> +++ b/drivers/staging/mt7621-pinctrl/pinctrl-rt2880.c
...

>  static int rt2880_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrldev,
>  					 struct device_node *np_config,
>  					 struct pinctrl_map **map,
>  					 unsigned int *num_maps)

num_maps passed in ( from dt_to_map_one_config in
drivers/pinctrl/devicetree.c)
is the address of an uninitialised variable - has the value -1681671408 in one test
run.


>  {
>  	struct rt2880_priv *p = pinctrl_dev_get_drvdata(pctrldev);
> +	struct property *prop;
> +	const char *function_name, *group_name;
>  	int ret;
> -	int max_maps = 0;
> +	int ngroups;
>  	unsigned int reserved_maps = 0;
> -	struct pinctrl_map *tmp;
> -	struct device_node *np;
>  
> -	for_each_child_of_node(np_config, np) {
> -		int ret = of_property_count_strings(np, "ralink,group");
> -
> -		if (ret >= 0)
> -			max_maps += ret;
> +	ngroups = of_property_count_strings(np_config, "groups");
> +	if (!ngroups) {
> +		dev_err(p->dev, "missing groups property in node %s\n",
> +			np_config->name);
> +		return -EINVAL;
>  	}
>  
> -	if (!max_maps)
> -		return max_maps;
> -
>  	ret = pinctrl_utils_reserve_map(pctrldev, map, &reserved_maps,
> -					num_maps, max_maps);
> +					num_maps, ngroups);

pinctrl_utils_reserve_map() expects *num_maps to be initialized here.
It does a memory allocation based on the value, and triggers a warning
for me:
	if (unlikely(order >= MAX_ORDER)) {
		WARN_ON_ONCE(!(gfp_mask & __GFP_NOWARN));
		return NULL;
	}


>  	if (ret) {
>  		dev_err(p->dev, "can't reserve map: %d\n", ret);
>  		return ret;
>  	}
>  
> -	tmp = *map;
> -
> -	for_each_child_of_node(np_config, np)
> -		rt2880_pinctrl_dt_subnode_to_map(pctrldev, np, &tmp);
> -	*num_maps = max_maps;

Previously *num_maps was initialised here - clearly too late.  Now it
isn't initialised at all.
Do you know what value it should be set to?

Thanks,
NeilBrown

Attachment: signature.asc
Description: PGP signature

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux