| 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
| ||
|
Date: Wed, 23 Feb 2022 09:51:46 +0800
From: Yicong Yang <yangyicong@...wei.com>
To: Shubhrajyoti Datta <shubhrajyoti.datta@...inx.com>,
<linux-i2c@...r.kernel.org>
CC: <yangyicong@...ilicon.com>, <git@...inx.com>,
<michal.simek@...inx.com>, <linux-kernel@...r.kernel.org>,
Chirag Parekh <chiragp@...inx.com>
Subject: Re: [PATCH] i2c: cadence: Recover bus after controller reset
On 2022/2/22 21:46, Shubhrajyoti Datta wrote:
> From: Chirag Parekh <chiragp@...inx.com>
>
> This will save from potential lock-up caused when I2c master controller
> resets in the middle of transfer and the slave is holding SDA line to
> transmit more data.
>
> Signed-off-by: Chirag Parekh <chiragp@...inx.com>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@...inx.com>
> ---
> drivers/i2c/busses/i2c-cadence.c | 109 +++++++++++++++++++++++++++++++
> 1 file changed, 109 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index 805c77143a0f..682821481b67 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
> @@ -7,13 +7,16 @@
>
> #include <linux/clk.h>
> #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/i2c.h>
> #include <linux/interrupt.h>
> #include <linux/io.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> #include <linux/of.h>
> +#include <linux/of_gpio.h>
> #include <linux/pm_runtime.h>
> +#include <linux/pinctrl/consumer.h>
>
> /* Register offsets for the I2C device. */
> #define CDNS_I2C_CR_OFFSET 0x00 /* Control Register, RW */
> @@ -179,6 +182,10 @@ enum cdns_i2c_slave_state {
> * @clk_rate_change_nb: Notifier block for clock rate changes
> * @quirks: flag for broken hold bit usage in r1p10
> * @ctrl_reg: Cached value of the control register.
> + * @rinfo: Structure holding recovery information.
> + * @pinctrl: Pin control state holder.
> + * @pinctrl_pins_default: Default pin control state.
> + * @pinctrl_pins_gpio: GPIO pin control state.
> * @ctrl_reg_diva_divb: value of fields DIV_A and DIV_B from CR register
> * @slave: Registered slave instance.
> * @dev_mode: I2C operating role(master/slave).
> @@ -204,6 +211,10 @@ struct cdns_i2c {
> struct notifier_block clk_rate_change_nb;
> u32 quirks;
> u32 ctrl_reg;
> + struct i2c_bus_recovery_info rinfo;
> + struct pinctrl *pinctrl;
> + struct pinctrl_state *pinctrl_pins_default;
> + struct pinctrl_state *pinctrl_pins_gpio;
Some reason for managing the pin info in the cdns_i2c struct? Since you're using
generic GPIO recovery, if you store these in struct i2c_bus_recovery_info the
i2c core framework will help handling the pin mux on recovery and I think you
don't need the prepare/unprepare() method anymore.
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> u16 ctrl_reg_diva_divb;
> struct i2c_client *slave;
> @@ -788,6 +799,7 @@ static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg *msg,
> /* Wait for the signal of completion */
> time_left = wait_for_completion_timeout(&id->xfer_done, adap->timeout);
> if (time_left == 0) {
> + i2c_recover_bus(adap);
> cdns_i2c_master_reset(adap);
> dev_err(id->adap.dev.parent,
> "timeout waiting on completion\n");
> @@ -1208,6 +1220,96 @@ static int __maybe_unused cdns_i2c_runtime_resume(struct device *dev)
> return 0;
> }
>
> +/**
> + * cdns_i2c_prepare_recovery - Withhold recovery state
> + * @adapter: Pointer to i2c adapter
> + *
> + * This function is called to prepare for recovery.
> + * It changes the state of pins from SCL/SDA to GPIO.
> + */
> +static void cdns_i2c_prepare_recovery(struct i2c_adapter *adapter)
> +{
> + struct cdns_i2c *p_cdns_i2c;
> + int ret;
> +
> + p_cdns_i2c = container_of(adapter, struct cdns_i2c, adap);
> +
> + /* Setting pin state as gpio */
> + ret = pinctrl_select_state(p_cdns_i2c->pinctrl,
> + p_cdns_i2c->pinctrl_pins_gpio);
> + if (ret < 0)
> + dev_err(p_cdns_i2c->adap.dev.parent,
> + "pinctrl_select_state failed\n");
> +}
> +
> +/**
> + * cdns_i2c_unprepare_recovery - Release recovery state
> + * @adapter: Pointer to i2c adapter
> + *
> + * This function is called on exiting recovery. It reverts
> + * the state of pins from GPIO to SCL/SDA.
> + */
> +static void cdns_i2c_unprepare_recovery(struct i2c_adapter *adapter)
> +{
> + struct cdns_i2c *p_cdns_i2c;
> + int ret;
> +
> + p_cdns_i2c = container_of(adapter, struct cdns_i2c, adap);
> +
> + /* Setting pin state to default(i2c) */
> + ret = pinctrl_select_state(p_cdns_i2c->pinctrl,
> + p_cdns_i2c->pinctrl_pins_default);
> + if (ret < 0)
> + dev_err(p_cdns_i2c->adap.dev.parent,
> + "pinctrl_select_state failed\n");
> +}
> +
> +/**
> + * cdns_i2c_init_recovery_info - Initialize I2C bus recovery
> + * @pid: Pointer to cdns i2c structure
> + * @pdev: Handle to the platform device structure
> + *
> + * This function does required initialization for i2c bus
> + * recovery. It registers three functions for prepare,
> + * recover and unprepare
> + *
> + * Return: 0 on Success, negative error otherwise.
> + */
> +static int cdns_i2c_init_recovery_info(struct cdns_i2c *pid,
> + struct platform_device *pdev)
> +{
> + struct i2c_bus_recovery_info *rinfo = &pid->rinfo;
> +
> + pid->pinctrl_pins_default = pinctrl_lookup_state(pid->pinctrl,
> + PINCTRL_STATE_DEFAULT);
> + pid->pinctrl_pins_gpio = pinctrl_lookup_state(pid->pinctrl, "gpio");
> +
> + /* Fetches GPIO pins */
> + rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda-gpios", GPIOD_ASIS);
> + rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl-gpios", GPIOD_ASIS);
> +
> + /* if GPIO driver isn't ready yet, deffer probe */
> + if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER ||
> + PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> +
> + /* Validates fetched information */
> + if (IS_ERR(rinfo->sda_gpiod) ||
> + IS_ERR(rinfo->scl_gpiod) ||
> + IS_ERR(pid->pinctrl_pins_default) ||
> + IS_ERR(pid->pinctrl_pins_gpio)) {
> + dev_dbg(&pdev->dev, "recovery information incomplete\n");
> + return 0;
> + }
> +
> + rinfo->prepare_recovery = cdns_i2c_prepare_recovery;
> + rinfo->unprepare_recovery = cdns_i2c_unprepare_recovery;
> + rinfo->recover_bus = i2c_generic_scl_recovery;
> + pid->adap.bus_recovery_info = rinfo;
> +
> + return 0;
> +}
> +
> static const struct dev_pm_ops cdns_i2c_dev_pm_ops = {
> SET_RUNTIME_PM_OPS(cdns_i2c_runtime_suspend,
> cdns_i2c_runtime_resume, NULL)
> @@ -1254,6 +1356,13 @@ static int cdns_i2c_probe(struct platform_device *pdev)
> id->quirks = data->quirks;
> }
>
> + id->pinctrl = devm_pinctrl_get(&pdev->dev);
> + if (!IS_ERR(id->pinctrl)) {
> + ret = cdns_i2c_init_recovery_info(id, pdev);
> + if (ret)
> + return ret;
> + }
> +
> id->membase = devm_platform_get_and_ioremap_resource(pdev, 0, &r_mem);
> if (IS_ERR(id->membase))
> return PTR_ERR(id->membase);
>
Powered by blists - more mailing lists