diff mbox series

tps6598x: clear int mask on probe failure

Message ID e6b80669-20f3-06e7-9ed5-8951a9c6db6f@kernel.dk
State New
Headers show
Series tps6598x: clear int mask on probe failure | expand

Commit Message

Jens Axboe Feb. 15, 2022, 6:22 p.m. UTC
The interrupt mask is enabled before any potential failure points in
the driver, which can leave a failure path where we exit with
interrupts enabled but the device not live. This causes an infinite
stream of interrupts on an Apple M1 Pro laptop on USB-C.

Add a failure label that's used post enabling interrupts, where we
mask them again before returning an error.

Suggested-by: Sven Peter <sven@svenpeter.dev>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/usb/typec/tipd/core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Heikki Krogerus Feb. 17, 2022, 8:30 a.m. UTC | #1
On Tue, Feb 15, 2022 at 11:22:04AM -0700, Jens Axboe wrote:
> The interrupt mask is enabled before any potential failure points in
> the driver, which can leave a failure path where we exit with
> interrupts enabled but the device not live. This causes an infinite
> stream of interrupts on an Apple M1 Pro laptop on USB-C.
> 
> Add a failure label that's used post enabling interrupts, where we
> mask them again before returning an error.
> 
> Suggested-by: Sven Peter <sven@svenpeter.dev>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>

Should this be marked as a fix?

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/core.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 6d27a5b5e3ca..7ffcda94d323 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -761,12 +761,12 @@ static int tps6598x_probe(struct i2c_client *client)
>  
>  	ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
>  	if (ret < 0)
> -		return ret;
> +		goto err_clear_mask;
>  	trace_tps6598x_status(status);
>  
>  	ret = tps6598x_read32(tps, TPS_REG_SYSTEM_CONF, &conf);
>  	if (ret < 0)
> -		return ret;
> +		goto err_clear_mask;
>  
>  	/*
>  	 * This fwnode has a "compatible" property, but is never populated as a
> @@ -855,7 +855,8 @@ static int tps6598x_probe(struct i2c_client *client)
>  	usb_role_switch_put(tps->role_sw);
>  err_fwnode_put:
>  	fwnode_handle_put(fwnode);
> -
> +err_clear_mask:
> +	tps6598x_write64(tps, TPS_REG_INT_MASK1, 0);
>  	return ret;
>  }
Jens Axboe Feb. 17, 2022, 1:51 p.m. UTC | #2
On 2/17/22 1:30 AM, Heikki Krogerus wrote:
> On Tue, Feb 15, 2022 at 11:22:04AM -0700, Jens Axboe wrote:
>> The interrupt mask is enabled before any potential failure points in
>> the driver, which can leave a failure path where we exit with
>> interrupts enabled but the device not live. This causes an infinite
>> stream of interrupts on an Apple M1 Pro laptop on USB-C.
>>
>> Add a failure label that's used post enabling interrupts, where we
>> mask them again before returning an error.
>>
>> Suggested-by: Sven Peter <sven@svenpeter.dev>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> Should this be marked as a fix?

Maybe, I can try and dig out the offending commit. From some quick
checking, it didn't come in with recent fixes, so it's probably been
there since support got added. Maybe we just mark it stable?

> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Thanks! Greg, will you pick this up?
Greg KH Feb. 17, 2022, 2:08 p.m. UTC | #3
On Thu, Feb 17, 2022 at 06:51:27AM -0700, Jens Axboe wrote:
> On 2/17/22 1:30 AM, Heikki Krogerus wrote:
> > On Tue, Feb 15, 2022 at 11:22:04AM -0700, Jens Axboe wrote:
> >> The interrupt mask is enabled before any potential failure points in
> >> the driver, which can leave a failure path where we exit with
> >> interrupts enabled but the device not live. This causes an infinite
> >> stream of interrupts on an Apple M1 Pro laptop on USB-C.
> >>
> >> Add a failure label that's used post enabling interrupts, where we
> >> mask them again before returning an error.
> >>
> >> Suggested-by: Sven Peter <sven@svenpeter.dev>
> >> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> > 
> > Should this be marked as a fix?
> 
> Maybe, I can try and dig out the offending commit. From some quick
> checking, it didn't come in with recent fixes, so it's probably been
> there since support got added. Maybe we just mark it stable?
> 
> > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> Thanks! Greg, will you pick this up?

Will do, thanks.

greg k-h
diff mbox series

Patch

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 6d27a5b5e3ca..7ffcda94d323 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -761,12 +761,12 @@  static int tps6598x_probe(struct i2c_client *client)
 
 	ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
 	if (ret < 0)
-		return ret;
+		goto err_clear_mask;
 	trace_tps6598x_status(status);
 
 	ret = tps6598x_read32(tps, TPS_REG_SYSTEM_CONF, &conf);
 	if (ret < 0)
-		return ret;
+		goto err_clear_mask;
 
 	/*
 	 * This fwnode has a "compatible" property, but is never populated as a
@@ -855,7 +855,8 @@  static int tps6598x_probe(struct i2c_client *client)
 	usb_role_switch_put(tps->role_sw);
 err_fwnode_put:
 	fwnode_handle_put(fwnode);
-
+err_clear_mask:
+	tps6598x_write64(tps, TPS_REG_INT_MASK1, 0);
 	return ret;
 }