lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZwVl_eaUO6twk1Fs@pengutronix.de>
Date: Tue, 8 Oct 2024 19:03:57 +0200
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Kory Maincent <kory.maincent@...tlin.com>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Jonathan Corbet <corbet@....net>,
	Donald Hunter <donald.hunter@...il.com>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	linux-doc@...r.kernel.org, Kyle Swenson <kyle.swenson@....tech>,
	Dent Project <dentproject@...uxfoundation.org>,
	kernel@...gutronix.de
Subject: Re: [PATCH net-next 12/12] net: pse-pd: tps23881: Add support for
 PSE events and interrupts

On Wed, Oct 02, 2024 at 06:28:08PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@...tlin.com>
> 
> Add support for PSE event reporting through interrupts. Set up the newly
> introduced devm_pse_irq_helper helper to register the interrupt. Events are
> reported for over-current and over-temperature conditions.
> 
> This patch also adds support for an OSS GPIO line to turn off all low
> priority ports in case of an over-current event.
> 
> Signed-off-by: Kory Maincent <kory.maincent@...tlin.com>
> ---
>  drivers/net/pse-pd/tps23881.c | 123 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 122 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
> index ddb44a17218a..03f36b641bb4 100644
> --- a/drivers/net/pse-pd/tps23881.c
> +++ b/drivers/net/pse-pd/tps23881.c
> @@ -17,6 +17,13 @@
>  
>  #define TPS23881_MAX_CHANS 8
>  
> +#define TPS23881_REG_IT		0x0
> +#define TPS23881_REG_IT_MASK	0x1
> +#define TPS23881_REG_IT_IFAULT	BIT(5)
> +#define TPS23881_REG_IT_SUPF	BIT(7)
> +#define TPS23881_REG_FAULT	0x7
> +#define TPS23881_REG_SUPF_EVENT	0xb
> +#define TPS23881_REG_TSD	BIT(7)
>  #define TPS23881_REG_PW_STATUS	0x10
>  #define TPS23881_REG_OP_MODE	0x12
>  #define TPS23881_OP_MODE_SEMIAUTO	0xaaaa
> @@ -25,6 +32,7 @@
>  #define TPS23881_REG_PW_PRIO	0x15
>  #define TPS23881_REG_GEN_MASK	0x17
>  #define TPS23881_REG_NBITACC	BIT(5)
> +#define TPS23881_REG_INTEN	BIT(7)
>  #define TPS23881_REG_PW_EN	0x19
>  #define TPS23881_REG_2PAIR_POL1	0x1e
>  #define TPS23881_REG_PORT_MAP	0x26
> @@ -59,6 +67,7 @@ struct tps23881_priv {
>  	struct pse_controller_dev pcdev;
>  	struct device_node *np;
>  	struct tps23881_port_desc port[TPS23881_MAX_CHANS];
> +	struct gpio_desc *oss;
>  };
>  
>  static struct tps23881_priv *to_tps23881_priv(struct pse_controller_dev *pcdev)
> @@ -1088,11 +1097,112 @@ static int tps23881_flash_sram_fw(struct i2c_client *client)
>  	return 0;
>  }
>  
> +static void tps23881_turn_off_low_prio(struct tps23881_priv *priv)
> +{
> +	dev_info(&priv->client->dev,
> +		 "turn off low priority ports due to over current event.\n");
> +	gpiod_set_value_cansleep(priv->oss, 1);
> +
> +	/* TPS23880 datasheet (Rev G) indicates minimum OSS pulse is 5us */
> +	usleep_range(5, 10);
> +	gpiod_set_value_cansleep(priv->oss, 0);

Ah, now I understand why 1 bit priority mode is used. The "fast" shutdown
path is done over interrupt and gpio bitbang.

It is not your fault...

> +}
> +
> +static int tps23881_irq_handler(int irq, struct pse_irq_data *pid,
> +				unsigned long *dev_mask)
> +{
> +	struct tps23881_priv *priv = (struct tps23881_priv *)pid->data;
> +	struct i2c_client *client = priv->client;
> +	struct pse_err_state *stat;
> +	int ret, i;
> +	u16 val;
> +
> +	*dev_mask = 0;
> +	for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> +		stat = &pid->states[i];
> +		stat->notifs = 0;
> +		stat->errors = 0;
> +	}
> +

Please add comment that two registers are read here in one run.

> +	ret = i2c_smbus_read_word_data(client, TPS23881_REG_IT);
> +	if (ret < 0)
> +		return PSE_FAILED_RETRY;
> +
> +	val = (u16)ret;
> +	if (val & TPS23881_REG_IT_SUPF) {
> +		ret = i2c_smbus_read_word_data(client, TPS23881_REG_SUPF_EVENT);
> +		if (ret < 0)
> +			return PSE_FAILED_RETRY;
> +
> +		if (ret & TPS23881_REG_TSD) {
> +			for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> +				stat = &pid->states[i];
> +				*dev_mask |= 1 << i;
> +				stat->notifs = PSE_EVENT_OVER_TEMP;
> +				stat->errors = PSE_ERROR_OVER_TEMP;
> +			}
> +		}
> +	}
> +
> +	if (val & (TPS23881_REG_IT_IFAULT | TPS23881_REG_IT_IFAULT << 8)) {

Ok, i see, you are reading two registers in one run and wont to test if
mask and status bits are active. In this code you will get true every
time the interrupt handler is executed.

> +		ret = i2c_smbus_read_word_data(client, TPS23881_REG_FAULT);
> +		if (ret < 0)
> +			return PSE_FAILED_RETRY;
> +
> +		val = (u16)(ret & 0xf0f);
> +
> +		/* Power cut detected, shutdown low priority port */
> +		if (val && priv->oss)
> +			tps23881_turn_off_low_prio(priv);
> +
> +		*dev_mask |= val;
> +		for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> +			if (val & BIT(i)) {
> +				stat = &pid->states[i];
> +				stat->notifs = PSE_EVENT_OVER_CURRENT;
> +				stat->errors = PSE_ERROR_OVER_CURRENT;
> +			}
> +		}
> +	}
> +
> +	return PSE_ERROR_CLEARED;
> +}
> +
> +static int tps23881_setup_irq(struct tps23881_priv *priv, int irq)
> +{
> +	int errs = PSE_ERROR_OVER_CURRENT | PSE_ERROR_OVER_TEMP;
> +	struct i2c_client *client = priv->client;
> +	struct pse_irq_desc irq_desc = {
> +		.name = "tps23881-irq",
> +		.map_event = tps23881_irq_handler,
> +		.data = priv,
> +	};
> +	int ret;
> +	u16 val;
> +
> +	val = TPS23881_REG_IT_IFAULT | TPS23881_REG_IT_SUPF |
> +	      TPS23881_REG_IT_IFAULT << 8 | TPS23881_REG_IT_SUPF << 8;
> +	ret = i2c_smbus_write_word_data(client, TPS23881_REG_IT_MASK, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_smbus_read_word_data(client, TPS23881_REG_GEN_MASK);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = (u16)(ret | TPS23881_REG_INTEN | TPS23881_REG_INTEN << 8);
> +	ret = i2c_smbus_write_word_data(client, TPS23881_REG_GEN_MASK, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return devm_pse_irq_helper(&priv->pcdev, irq, 0, errs, &irq_desc);
> +}
> +
>  static int tps23881_i2c_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
>  	struct tps23881_priv *priv;
> -	struct gpio_desc *reset;
> +	struct gpio_desc *reset, *oss;
>  	int ret;
>  	u8 val;
>  
> @@ -1169,6 +1279,17 @@ static int tps23881_i2c_probe(struct i2c_client *client)
>  				     "failed to register PSE controller\n");
>  	}
>  
> +	oss = devm_gpiod_get_optional(dev, "oss", GPIOD_OUT_LOW);
> +	if (IS_ERR(oss))
> +		return dev_err_probe(&client->dev, PTR_ERR(oss), "Failed to get OSS GPIO\n");
> +	priv->oss = oss;
> +
> +	if (client->irq) {
> +		ret = tps23881_setup_irq(priv, client->irq);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return ret;
>  }
>  
> 
> -- 
> 2.34.1
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ