| Message ID | 1579675994-7001-1-git-send-email-light.hsieh@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v8,1/6] pinctrl: mediatek: Check gpio pin number and use binary search in mtk_hw_pin_field_lookup() | expand |
Dear Sean, V8 uploaded according to your review comment on V7. Please take time to review. Thanks. Light On Wed, 2020-01-22 at 14:53 +0800, light.hsieh@mediatek.com wrote: > From: Light Hsieh <light.hsieh@mediatek.com> > > 1. Check if gpio pin number is in valid range to prevent from get invalid > pointer 'desc' in the following code: > desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio]; > > 2. Improve mtk_hw_pin_field_lookup() > 2.1 Modify mtk_hw_pin_field_lookup() to use binary search for accelerating > search. > 2.2 Correct message after the following check fail: > if (hw->soc->reg_cal && hw->soc->reg_cal[field].range) { > rc = &hw->soc->reg_cal[field]; > The original message is: > "Not support field %d for pin %d (%s)\n" > However, the check is on soc chip level, not on pin level yet. > So the message is corrected as: > "Not support field %d for this soc\n" > > Signed-off-by: Light Hsieh <light.hsieh@mediatek.com> > --- > drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c | 27 ++++++++++++++++++------ > drivers/pinctrl/mediatek/pinctrl-paris.c | 25 ++++++++++++++++++++++ > 2 files changed, 46 insertions(+), 6 deletions(-) > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c > index 20e1c89..d63e05e 100644 > --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c > +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c > @@ -68,32 +68,44 @@ static int mtk_hw_pin_field_lookup(struct mtk_pinctrl *hw, > { > const struct mtk_pin_field_calc *c, *e; > const struct mtk_pin_reg_calc *rc; > + int start = 0, end, check; > + bool found = false; > u32 bits; > > if (hw->soc->reg_cal && hw->soc->reg_cal[field].range) { > rc = &hw->soc->reg_cal[field]; > } else { > dev_dbg(hw->dev, > - "Not support field %d for pin %d (%s)\n", > - field, desc->number, desc->name); > + "Not support field %d for this soc\n", field); > return -ENOTSUPP; > } > > + end = rc->nranges - 1; > c = rc->range; > e = c + rc->nranges; > > - while (c < e) { > - if (desc->number >= c->s_pin && desc->number <= c->e_pin) > + while (start <= end) { > + check = (start + end) >> 1; > + if (desc->number >= rc->range[check].s_pin > + && desc->number <= rc->range[check].e_pin) { > + found = true; > + break; > + } else if (start == end) > break; > - c++; > + else if (desc->number < rc->range[check].s_pin) > + end = check - 1; > + else > + start = check + 1; > } > > - if (c >= e) { > + if (!found) { > dev_dbg(hw->dev, "Not support field %d for pin = %d (%s)\n", > field, desc->number, desc->name); > return -ENOTSUPP; > } > > + c = rc->range + check; > + > if (c->i_base > hw->nbase - 1) { > dev_err(hw->dev, > "Invalid base for field %d for pin = %d (%s)\n", > @@ -182,6 +194,9 @@ int mtk_hw_set_value(struct mtk_pinctrl *hw, const struct mtk_pin_desc *desc, > if (err) > return err; > > + if (value < 0 || value > pf.mask) > + return -EINVAL; > + > if (!pf.next) > mtk_rmw(hw, pf.index, pf.offset, pf.mask << pf.bitpos, > (value & pf.mask) << pf.bitpos); > diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c > index 923264d..3e13ae7 100644 > --- a/drivers/pinctrl/mediatek/pinctrl-paris.c > +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c > @@ -81,6 +81,8 @@ static int mtk_pinconf_get(struct pinctrl_dev *pctldev, > int val, val2, err, reg, ret = 1; > const struct mtk_pin_desc *desc; > > + if (pin >= hw->soc->npins) > + return -EINVAL; > desc = (const struct mtk_pin_desc *)&hw->soc->pins[pin]; > > switch (param) { > @@ -206,6 +208,10 @@ static int mtk_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin, > int err = 0; > u32 reg; > > + if (pin >= hw->soc->npins) { > + err = -EINVAL; > + goto err; > + } > desc = (const struct mtk_pin_desc *)&hw->soc->pins[pin]; > > switch ((u32)param) { > @@ -693,6 +699,9 @@ static int mtk_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio) > const struct mtk_pin_desc *desc; > int value, err; > > + if (gpio > hw->soc->npins) > + return -EINVAL; > + > desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio]; > > err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DIR, &value); > @@ -708,6 +717,9 @@ static int mtk_gpio_get(struct gpio_chip *chip, unsigned int gpio) > const struct mtk_pin_desc *desc; > int value, err; > > + if (gpio > hw->soc->npins) > + return -EINVAL; > + > desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio]; > > err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DI, &value); > @@ -722,6 +734,9 @@ static void mtk_gpio_set(struct gpio_chip *chip, unsigned int gpio, int value) > struct mtk_pinctrl *hw = gpiochip_get_data(chip); > const struct mtk_pin_desc *desc; > > + if (gpio > hw->soc->npins) > + return; > + > desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio]; > > mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_DO, !!value); > @@ -729,12 +744,22 @@ static void mtk_gpio_set(struct gpio_chip *chip, unsigned int gpio, int value) > > static int mtk_gpio_direction_input(struct gpio_chip *chip, unsigned int gpio) > { > + struct mtk_pinctrl *hw = gpiochip_get_data(chip); > + > + if (gpio > hw->soc->npins) > + return -EINVAL; > + > return pinctrl_gpio_direction_input(chip->base + gpio); > } > > static int mtk_gpio_direction_output(struct gpio_chip *chip, unsigned int gpio, > int value) > { > + struct mtk_pinctrl *hw = gpiochip_get_data(chip); > + > + if (gpio > hw->soc->npins) > + return -EINVAL; > + > mtk_gpio_set(chip, gpio, value); > > return pinctrl_gpio_direction_output(chip->base + gpio);
On Tue, Jan 21, 2020 at 10:53 PM <light.hsieh@mediatek.com> wrote: > > From: Light Hsieh <light.hsieh@mediatek.com> > > 1. Check if gpio pin number is in valid range to prevent from get invalid > pointer 'desc' in the following code: > desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio]; > > 2. Improve mtk_hw_pin_field_lookup() > 2.1 Modify mtk_hw_pin_field_lookup() to use binary search for accelerating > search. > 2.2 Correct message after the following check fail: > if (hw->soc->reg_cal && hw->soc->reg_cal[field].range) { > rc = &hw->soc->reg_cal[field]; > The original message is: > "Not support field %d for pin %d (%s)\n" > However, the check is on soc chip level, not on pin level yet. > So the message is corrected as: > "Not support field %d for this soc\n" > > Signed-off-by: Light Hsieh <light.hsieh@mediatek.com> Acked-by: Sean Wang <sean.wang@kernel.org> > --- > drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c | 27 ++++++++++++++++++------ > drivers/pinctrl/mediatek/pinctrl-paris.c | 25 ++++++++++++++++++++++ > 2 files changed, 46 insertions(+), 6 deletions(-) > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c > index 20e1c89..d63e05e 100644 > --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c > +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c > @@ -68,32 +68,44 @@ static int mtk_hw_pin_field_lookup(struct mtk_pinctrl *hw, > { > const struct mtk_pin_field_calc *c, *e; > const struct mtk_pin_reg_calc *rc; > + int start = 0, end, check; > + bool found = false; > u32 bits; > > if (hw->soc->reg_cal && hw->soc->reg_cal[field].range) { > rc = &hw->soc->reg_cal[field]; > } else { > dev_dbg(hw->dev, > - "Not support field %d for pin %d (%s)\n", > - field, desc->number, desc->name); > + "Not support field %d for this soc\n", field); > return -ENOTSUPP; > } > > + end = rc->nranges - 1; > c = rc->range; > e = c + rc->nranges; > > - while (c < e) { > - if (desc->number >= c->s_pin && desc->number <= c->e_pin) > + while (start <= end) { > + check = (start + end) >> 1; > + if (desc->number >= rc->range[check].s_pin > + && desc->number <= rc->range[check].e_pin) { > + found = true; > + break; > + } else if (start == end) > break; > - c++; > + else if (desc->number < rc->range[check].s_pin) > + end = check - 1; > + else > + start = check + 1; > } > > - if (c >= e) { > + if (!found) { > dev_dbg(hw->dev, "Not support field %d for pin = %d (%s)\n", > field, desc->number, desc->name); > return -ENOTSUPP; > } > > + c = rc->range + check; > + > if (c->i_base > hw->nbase - 1) { > dev_err(hw->dev, > "Invalid base for field %d for pin = %d (%s)\n", > @@ -182,6 +194,9 @@ int mtk_hw_set_value(struct mtk_pinctrl *hw, const struct mtk_pin_desc *desc, > if (err) > return err; > > + if (value < 0 || value > pf.mask) > + return -EINVAL; > + > if (!pf.next) > mtk_rmw(hw, pf.index, pf.offset, pf.mask << pf.bitpos, > (value & pf.mask) << pf.bitpos); > diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c > index 923264d..3e13ae7 100644 > --- a/drivers/pinctrl/mediatek/pinctrl-paris.c > +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c > @@ -81,6 +81,8 @@ static int mtk_pinconf_get(struct pinctrl_dev *pctldev, > int val, val2, err, reg, ret = 1; > const struct mtk_pin_desc *desc; > > + if (pin >= hw->soc->npins) > + return -EINVAL; > desc = (const struct mtk_pin_desc *)&hw->soc->pins[pin]; > > switch (param) { > @@ -206,6 +208,10 @@ static int mtk_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin, > int err = 0; > u32 reg; > > + if (pin >= hw->soc->npins) { > + err = -EINVAL; > + goto err; > + } > desc = (const struct mtk_pin_desc *)&hw->soc->pins[pin]; > > switch ((u32)param) { > @@ -693,6 +699,9 @@ static int mtk_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio) > const struct mtk_pin_desc *desc; > int value, err; > > + if (gpio > hw->soc->npins) > + return -EINVAL; > + > desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio]; > > err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DIR, &value); > @@ -708,6 +717,9 @@ static int mtk_gpio_get(struct gpio_chip *chip, unsigned int gpio) > const struct mtk_pin_desc *desc; > int value, err; > > + if (gpio > hw->soc->npins) > + return -EINVAL; > + > desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio]; > > err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DI, &value); > @@ -722,6 +734,9 @@ static void mtk_gpio_set(struct gpio_chip *chip, unsigned int gpio, int value) > struct mtk_pinctrl *hw = gpiochip_get_data(chip); > const struct mtk_pin_desc *desc; > > + if (gpio > hw->soc->npins) > + return; > + > desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio]; > > mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_DO, !!value); > @@ -729,12 +744,22 @@ static void mtk_gpio_set(struct gpio_chip *chip, unsigned int gpio, int value) > > static int mtk_gpio_direction_input(struct gpio_chip *chip, unsigned int gpio) > { > + struct mtk_pinctrl *hw = gpiochip_get_data(chip); > + > + if (gpio > hw->soc->npins) > + return -EINVAL; > + > return pinctrl_gpio_direction_input(chip->base + gpio); > } > > static int mtk_gpio_direction_output(struct gpio_chip *chip, unsigned int gpio, > int value) > { > + struct mtk_pinctrl *hw = gpiochip_get_data(chip); > + > + if (gpio > hw->soc->npins) > + return -EINVAL; > + > mtk_gpio_set(chip, gpio, value); > > return pinctrl_gpio_direction_output(chip->base + gpio); > -- > 1.8.1.1.dirty
On Wed, Jan 22, 2020 at 7:53 AM <light.hsieh@mediatek.com> wrote: > From: Light Hsieh <light.hsieh@mediatek.com> > > 1. Check if gpio pin number is in valid range to prevent from get invalid > pointer 'desc' in the following code: > desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio]; > > 2. Improve mtk_hw_pin_field_lookup() > 2.1 Modify mtk_hw_pin_field_lookup() to use binary search for accelerating > search. > 2.2 Correct message after the following check fail: > if (hw->soc->reg_cal && hw->soc->reg_cal[field].range) { > rc = &hw->soc->reg_cal[field]; > The original message is: > "Not support field %d for pin %d (%s)\n" > However, the check is on soc chip level, not on pin level yet. > So the message is corrected as: > "Not support field %d for this soc\n" > > Signed-off-by: Light Hsieh <light.hsieh@mediatek.com> I managed to apply all 6 patches now for v5.7. I had a big problem extracting the patches since they get base64 encoded and for some reason git am cannot deal with this. I thought it would but it doesn't possibly because of custom headers in the message. I have to save out the base64 part of the message, decode separately, then paste back the result removing the transfer-encoding line of the original message. cat mtk65.txt | base64 -d -i > scratch.patch Any tips on how to handle this more efficiently? Yours, Linus Walleij
diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c index 20e1c89..d63e05e 100644 --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c @@ -68,32 +68,44 @@ static int mtk_hw_pin_field_lookup(struct mtk_pinctrl *hw, { const struct mtk_pin_field_calc *c, *e; const struct mtk_pin_reg_calc *rc; + int start = 0, end, check; + bool found = false; u32 bits; if (hw->soc->reg_cal && hw->soc->reg_cal[field].range) { rc = &hw->soc->reg_cal[field]; } else { dev_dbg(hw->dev, - "Not support field %d for pin %d (%s)\n", - field, desc->number, desc->name); + "Not support field %d for this soc\n", field); return -ENOTSUPP; } + end = rc->nranges - 1; c = rc->range; e = c + rc->nranges; - while (c < e) { - if (desc->number >= c->s_pin && desc->number <= c->e_pin) + while (start <= end) { + check = (start + end) >> 1; + if (desc->number >= rc->range[check].s_pin + && desc->number <= rc->range[check].e_pin) { + found = true; + break; + } else if (start == end) break; - c++; + else if (desc->number < rc->range[check].s_pin) + end = check - 1; + else + start = check + 1; } - if (c >= e) { + if (!found) { dev_dbg(hw->dev, "Not support field %d for pin = %d (%s)\n", field, desc->number, desc->name); return -ENOTSUPP; } + c = rc->range + check; + if (c->i_base > hw->nbase - 1) { dev_err(hw->dev, "Invalid base for field %d for pin = %d (%s)\n", @@ -182,6 +194,9 @@ int mtk_hw_set_value(struct mtk_pinctrl *hw, const struct mtk_pin_desc *desc, if (err) return err; + if (value < 0 || value > pf.mask) + return -EINVAL; + if (!pf.next) mtk_rmw(hw, pf.index, pf.offset, pf.mask << pf.bitpos, (value & pf.mask) << pf.bitpos); diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c index 923264d..3e13ae7 100644 --- a/drivers/pinctrl/mediatek/pinctrl-paris.c +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c @@ -81,6 +81,8 @@ static int mtk_pinconf_get(struct pinctrl_dev *pctldev, int val, val2, err, reg, ret = 1; const struct mtk_pin_desc *desc; + if (pin >= hw->soc->npins) + return -EINVAL; desc = (const struct mtk_pin_desc *)&hw->soc->pins[pin]; switch (param) { @@ -206,6 +208,10 @@ static int mtk_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin, int err = 0; u32 reg; + if (pin >= hw->soc->npins) { + err = -EINVAL; + goto err; + } desc = (const struct mtk_pin_desc *)&hw->soc->pins[pin]; switch ((u32)param) { @@ -693,6 +699,9 @@ static int mtk_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio) const struct mtk_pin_desc *desc; int value, err; + if (gpio > hw->soc->npins) + return -EINVAL; + desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio]; err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DIR, &value); @@ -708,6 +717,9 @@ static int mtk_gpio_get(struct gpio_chip *chip, unsigned int gpio) const struct mtk_pin_desc *desc; int value, err; + if (gpio > hw->soc->npins) + return -EINVAL; + desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio]; err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DI, &value); @@ -722,6 +734,9 @@ static void mtk_gpio_set(struct gpio_chip *chip, unsigned int gpio, int value) struct mtk_pinctrl *hw = gpiochip_get_data(chip); const struct mtk_pin_desc *desc; + if (gpio > hw->soc->npins) + return; + desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio]; mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_DO, !!value); @@ -729,12 +744,22 @@ static void mtk_gpio_set(struct gpio_chip *chip, unsigned int gpio, int value) static int mtk_gpio_direction_input(struct gpio_chip *chip, unsigned int gpio) { + struct mtk_pinctrl *hw = gpiochip_get_data(chip); + + if (gpio > hw->soc->npins) + return -EINVAL; + return pinctrl_gpio_direction_input(chip->base + gpio); } static int mtk_gpio_direction_output(struct gpio_chip *chip, unsigned int gpio, int value) { + struct mtk_pinctrl *hw = gpiochip_get_data(chip); + + if (gpio > hw->soc->npins) + return -EINVAL; + mtk_gpio_set(chip, gpio, value); return pinctrl_gpio_direction_output(chip->base + gpio);