Both anx78xx_set_bits() and anx78xx_clear_bits() in the poweron process
may fail. The fix inserts checks for their return values. If the poweron
process fails, it calls anx78xx_poweroff().
Signed-off-by: Kangjie Lu <[email protected]>
---
drivers/gpu/drm/bridge/analogix-anx78xx.c | 26 ++++++++++++++++-------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c
index f8433c93f463..a57104c71739 100644
--- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
+++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
@@ -610,20 +610,20 @@ static int anx78xx_enable_interrupts(struct anx78xx *anx78xx)
return 0;
}
-static void anx78xx_poweron(struct anx78xx *anx78xx)
+static int anx78xx_poweron(struct anx78xx *anx78xx)
{
struct anx78xx_platform_data *pdata = &anx78xx->pdata;
- int err;
+ int err = 0;
if (WARN_ON(anx78xx->powered))
- return;
+ return err;
if (pdata->dvdd10) {
err = regulator_enable(pdata->dvdd10);
if (err) {
DRM_ERROR("Failed to enable DVDD10 regulator: %d\n",
err);
- return;
+ return err;
}
usleep_range(1000, 2000);
@@ -638,12 +638,18 @@ static void anx78xx_poweron(struct anx78xx *anx78xx)
gpiod_set_value_cansleep(pdata->gpiod_reset, 0);
/* Power on registers module */
- anx78xx_set_bits(anx78xx->map[I2C_IDX_TX_P2], SP_POWERDOWN_CTRL_REG,
+ err = anx78xx_set_bits(anx78xx->map[I2C_IDX_TX_P2], SP_POWERDOWN_CTRL_REG,
SP_HDCP_PD | SP_AUDIO_PD | SP_VIDEO_PD | SP_LINK_PD);
- anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2], SP_POWERDOWN_CTRL_REG,
+ err |= anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2], SP_POWERDOWN_CTRL_REG,
SP_REGISTER_PD | SP_TOTAL_PD);
+ if (err) {
+ anx78xx_poweroff(anx78xx);
+ return err;
+ }
anx78xx->powered = true;
+
+ return err;
}
static void anx78xx_poweroff(struct anx78xx *anx78xx)
@@ -1144,7 +1150,9 @@ static irqreturn_t anx78xx_hpd_threaded_handler(int irq, void *data)
mutex_lock(&anx78xx->lock);
/* Cable is pulled, power on the chip */
- anx78xx_poweron(anx78xx);
+ err = anx78xx_poweron(anx78xx);
+ if (err)
+ DRM_ERROR("Failed to power on the chip: %d\n", err);
err = anx78xx_enable_interrupts(anx78xx);
if (err)
@@ -1379,7 +1387,9 @@ static int anx78xx_i2c_probe(struct i2c_client *client,
}
/* Look for supported chip ID */
- anx78xx_poweron(anx78xx);
+ err = anx78xx_poweron(anx78xx);
+ if (err)
+ goto err_poweroff;
err = regmap_read(anx78xx->map[I2C_IDX_TX_P2], SP_DEVICE_IDL_REG,
&idl);
--
2.17.2 (Apple Git-113)
Hi Kangjie,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.20-rc7 next-20181220]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Kangjie-Lu/gpu-anx7808-fix-a-missing-check-of-return-value/20181221-054313
config: nds32-allmodconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 6.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=6.4.0 make.cross ARCH=nds32
All errors (new ones prefixed by >>):
drivers/gpu/drm/bridge/analogix-anx78xx.c: In function 'anx78xx_poweron':
>> drivers/gpu/drm/bridge/analogix-anx78xx.c:646:3: error: implicit declaration of function 'anx78xx_poweroff' [-Werror=implicit-function-declaration]
anx78xx_poweroff(anx78xx);
^~~~~~~~~~~~~~~~
drivers/gpu/drm/bridge/analogix-anx78xx.c: At top level:
drivers/gpu/drm/bridge/analogix-anx78xx.c:655:13: warning: conflicting types for 'anx78xx_poweroff'
static void anx78xx_poweroff(struct anx78xx *anx78xx)
^~~~~~~~~~~~~~~~
drivers/gpu/drm/bridge/analogix-anx78xx.c:655:13: error: static declaration of 'anx78xx_poweroff' follows non-static declaration
drivers/gpu/drm/bridge/analogix-anx78xx.c:646:3: note: previous implicit declaration of 'anx78xx_poweroff' was here
anx78xx_poweroff(anx78xx);
^~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/anx78xx_poweroff +646 drivers/gpu/drm/bridge/analogix-anx78xx.c
612
613 static int anx78xx_poweron(struct anx78xx *anx78xx)
614 {
615 struct anx78xx_platform_data *pdata = &anx78xx->pdata;
616 int err = 0;
617
618 if (WARN_ON(anx78xx->powered))
619 return err;
620
621 if (pdata->dvdd10) {
622 err = regulator_enable(pdata->dvdd10);
623 if (err) {
624 DRM_ERROR("Failed to enable DVDD10 regulator: %d\n",
625 err);
626 return err;
627 }
628
629 usleep_range(1000, 2000);
630 }
631
632 gpiod_set_value_cansleep(pdata->gpiod_reset, 1);
633 usleep_range(1000, 2000);
634
635 gpiod_set_value_cansleep(pdata->gpiod_pd, 0);
636 usleep_range(1000, 2000);
637
638 gpiod_set_value_cansleep(pdata->gpiod_reset, 0);
639
640 /* Power on registers module */
641 err = anx78xx_set_bits(anx78xx->map[I2C_IDX_TX_P2], SP_POWERDOWN_CTRL_REG,
642 SP_HDCP_PD | SP_AUDIO_PD | SP_VIDEO_PD | SP_LINK_PD);
643 err |= anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2], SP_POWERDOWN_CTRL_REG,
644 SP_REGISTER_PD | SP_TOTAL_PD);
645 if (err) {
> 646 anx78xx_poweroff(anx78xx);
647 return err;
648 }
649
650 anx78xx->powered = true;
651
652 return err;
653 }
654
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Kangjie,
Thank you for the patch.
On Thursday, 20 December 2018 09:41:16 EET Kangjie Lu wrote:
> Both anx78xx_set_bits() and anx78xx_clear_bits() in the poweron process
> may fail. The fix inserts checks for their return values. If the poweron
> process fails, it calls anx78xx_poweroff().
>
> Signed-off-by: Kangjie Lu <[email protected]>
> ---
> drivers/gpu/drm/bridge/analogix-anx78xx.c | 26 ++++++++++++++++-------
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c
> b/drivers/gpu/drm/bridge/analogix-anx78xx.c index
> f8433c93f463..a57104c71739 100644
> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
> @@ -610,20 +610,20 @@ static int anx78xx_enable_interrupts(struct anx78xx
> *anx78xx) return 0;
> }
>
> -static void anx78xx_poweron(struct anx78xx *anx78xx)
> +static int anx78xx_poweron(struct anx78xx *anx78xx)
> {
> struct anx78xx_platform_data *pdata = &anx78xx->pdata;
> - int err;
> + int err = 0;
>
> if (WARN_ON(anx78xx->powered))
> - return;
> + return err;
You can return 0 here.
>
> if (pdata->dvdd10) {
> err = regulator_enable(pdata->dvdd10);
> if (err) {
> DRM_ERROR("Failed to enable DVDD10 regulator: %d\n",
> err);
> - return;
> + return err;
> }
>
> usleep_range(1000, 2000);
> @@ -638,12 +638,18 @@ static void anx78xx_poweron(struct anx78xx *anx78xx)
> gpiod_set_value_cansleep(pdata->gpiod_reset, 0);
>
> /* Power on registers module */
> - anx78xx_set_bits(anx78xx->map[I2C_IDX_TX_P2], SP_POWERDOWN_CTRL_REG,
> + err = anx78xx_set_bits(anx78xx->map[I2C_IDX_TX_P2], SP_POWERDOWN_CTRL_REG,
> SP_HDCP_PD | SP_AUDIO_PD | SP_VIDEO_PD | SP_LINK_PD);
> - anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2], SP_POWERDOWN_CTRL_REG,
> + err |= anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2],
> SP_POWERDOWN_CTRL_REG, SP_REGISTER_PD | SP_TOTAL_PD);
If both functions fail with a different error code, this may result in a
meaningless error being returned. One option is to do
err = anx78xx_set_bits(...);
if (!err)
err = anx78xx_clear_bits(...);
The construct gets quickly ugly though, so I sometimes define register
accessors as taking an int * for the error code instead of returning it.
void write(..., int *status)
{
if (*status)
return;
*status = real_write(...);
}
and then use it as
int status = 0;
write(..., &status);
write(..., &status);
write(..., &status);
write(..., &status);
write(..., &status);
return status;
This may be overkill here.
> + if (err) {
> + anx78xx_poweroff(anx78xx);
> + return err;
> + }
>
> anx78xx->powered = true;
> +
> + return err;
And return 0 here too, removing the need to initialize the err variable to 0.
> }
>
> static void anx78xx_poweroff(struct anx78xx *anx78xx)
> @@ -1144,7 +1150,9 @@ static irqreturn_t anx78xx_hpd_threaded_handler(int
> irq, void *data) mutex_lock(&anx78xx->lock);
>
> /* Cable is pulled, power on the chip */
> - anx78xx_poweron(anx78xx);
> + err = anx78xx_poweron(anx78xx);
> + if (err)
> + DRM_ERROR("Failed to power on the chip: %d\n", err);
Wouldn't it be better to move the error message to the an78xx_poweron()
function ? That way it would also be printed in the probe() path.
> err = anx78xx_enable_interrupts(anx78xx);
> if (err)
> @@ -1379,7 +1387,9 @@ static int anx78xx_i2c_probe(struct i2c_client
> *client, }
>
> /* Look for supported chip ID */
> - anx78xx_poweron(anx78xx);
> + err = anx78xx_poweron(anx78xx);
> + if (err)
> + goto err_poweroff;
If poweron fails, doesn't it clean up after itself ? Do you need to call
poweroff here ?
> err = regmap_read(anx78xx->map[I2C_IDX_TX_P2], SP_DEVICE_IDL_REG,
> &idl);
--
Regards,
Laurent Pinchart
Hi Kangjie,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.20-rc7 next-20181220]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Kangjie-Lu/gpu-anx7808-fix-a-missing-check-of-return-value/20181221-054313
config: x86_64-randconfig-x013-201850 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
drivers/gpu/drm/bridge/analogix-anx78xx.c: In function 'anx78xx_poweron':
>> drivers/gpu/drm/bridge/analogix-anx78xx.c:646:3: error: implicit declaration of function 'anx78xx_poweroff'; did you mean 'anx78xx_poweron'? [-Werror=implicit-function-declaration]
anx78xx_poweroff(anx78xx);
^~~~~~~~~~~~~~~~
anx78xx_poweron
drivers/gpu/drm/bridge/analogix-anx78xx.c: At top level:
>> drivers/gpu/drm/bridge/analogix-anx78xx.c:655:13: warning: conflicting types for 'anx78xx_poweroff'
static void anx78xx_poweroff(struct anx78xx *anx78xx)
^~~~~~~~~~~~~~~~
>> drivers/gpu/drm/bridge/analogix-anx78xx.c:655:13: error: static declaration of 'anx78xx_poweroff' follows non-static declaration
drivers/gpu/drm/bridge/analogix-anx78xx.c:646:3: note: previous implicit declaration of 'anx78xx_poweroff' was here
anx78xx_poweroff(anx78xx);
^~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +646 drivers/gpu/drm/bridge/analogix-anx78xx.c
612
613 static int anx78xx_poweron(struct anx78xx *anx78xx)
614 {
615 struct anx78xx_platform_data *pdata = &anx78xx->pdata;
616 int err = 0;
617
618 if (WARN_ON(anx78xx->powered))
619 return err;
620
621 if (pdata->dvdd10) {
622 err = regulator_enable(pdata->dvdd10);
623 if (err) {
624 DRM_ERROR("Failed to enable DVDD10 regulator: %d\n",
625 err);
626 return err;
627 }
628
629 usleep_range(1000, 2000);
630 }
631
632 gpiod_set_value_cansleep(pdata->gpiod_reset, 1);
633 usleep_range(1000, 2000);
634
635 gpiod_set_value_cansleep(pdata->gpiod_pd, 0);
636 usleep_range(1000, 2000);
637
638 gpiod_set_value_cansleep(pdata->gpiod_reset, 0);
639
640 /* Power on registers module */
641 err = anx78xx_set_bits(anx78xx->map[I2C_IDX_TX_P2], SP_POWERDOWN_CTRL_REG,
642 SP_HDCP_PD | SP_AUDIO_PD | SP_VIDEO_PD | SP_LINK_PD);
643 err |= anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2], SP_POWERDOWN_CTRL_REG,
644 SP_REGISTER_PD | SP_TOTAL_PD);
645 if (err) {
> 646 anx78xx_poweroff(anx78xx);
647 return err;
648 }
649
650 anx78xx->powered = true;
651
652 return err;
653 }
654
> 655 static void anx78xx_poweroff(struct anx78xx *anx78xx)
656 {
657 struct anx78xx_platform_data *pdata = &anx78xx->pdata;
658 int err;
659
660 if (WARN_ON(!anx78xx->powered))
661 return;
662
663 gpiod_set_value_cansleep(pdata->gpiod_reset, 1);
664 usleep_range(1000, 2000);
665
666 gpiod_set_value_cansleep(pdata->gpiod_pd, 1);
667 usleep_range(1000, 2000);
668
669 if (pdata->dvdd10) {
670 err = regulator_disable(pdata->dvdd10);
671 if (err) {
672 DRM_ERROR("Failed to disable DVDD10 regulator: %d\n",
673 err);
674 return;
675 }
676
677 usleep_range(1000, 2000);
678 }
679
680 anx78xx->powered = false;
681 }
682
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation