| 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
| ||
|
Message-ID: <Yoi1bV95l7+thgS5@shikoro>
Date: Sat, 21 May 2022 11:48:29 +0200
From: Wolfram Sang <wsa@...nel.org>
To: Conor Dooley <conor.dooley@...rochip.com>
Cc: linux-i2c@...r.kernel.org, ben.dooks@...ethink.co.uk,
linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
daire.mcnamara@...rochip.com
Subject: Re: [PATCH v3] i2c: add support for microchip fpga i2c controllers
Hi Conor,
driver looks mostly good, but some comments:
> +/*
> + * mchp_corei2c_dev - I2C device context
> + * @base: pointer to register struct
> + * @msg: pointer to current message
> + * @msg_len: number of bytes transferred in msg
> + * @msg_err: error code for completed message
> + * @msg_complete: xfer completion object
> + * @dev: device reference
> + * @adapter: core i2c abstraction
> + * @i2c_clk: clock reference for i2c input clock
> + * @bus_clk_rate: current i2c bus clock rate
> + * @buf: ptr to msg buffer for easier use.
> + * @isr_status: cached copy of local ISR status.
> + * @lock: spinlock for IRQ synchronization.
> + */
This seems outdated, e.g. msg is not in the struct but addr is missing.
Also, proper kdoc header is missing?
> +struct mchp_corei2c_dev {
> + void __iomem *base;
> + size_t msg_len;
Maybe u16 to match the original type from struct i2c_msg?
> + int msg_err;
> + struct completion msg_complete;
> + struct device *dev;
> + struct i2c_adapter adapter;
> + struct clk *i2c_clk;
> + spinlock_t lock; /* IRQ synchronization */
> + u32 bus_clk_rate;
> + u32 msg_read;
This is initialized but never used?
> + u32 isr_status;
> + u8 *buf;
> + u8 addr;
> +};
> +
...
> +static int mchp_corei2c_init(struct mchp_corei2c_dev *idev)
> +{
> + u32 clk_rate = clk_get_rate(idev->i2c_clk);
> + u32 divisor = clk_rate / idev->bus_clk_rate;
I don't see a protection against division by zero?
> + int ret;
> +
> + ret = mchp_corei2c_set_divisor(divisor, idev);
> + if (ret)
> + return ret;
> +
> + mchp_corei2c_reset(idev);
> +
> + return 0;
> +}
> +
> +static void mchp_corei2c_transfer(struct mchp_corei2c_dev *idev, u32 data)
> +{
> + if (idev->msg_len > 0)
> + writeb(data, idev->base + CORE_I2C_DATA);
> +}
Minor: this is very finegrained and only called once. I'd put it into
the calling function.
...
> +static irqreturn_t mchp_corei2c_handle_isr(struct mchp_corei2c_dev *idev)
> +{
> + u32 status = idev->isr_status;
> + u8 ctrl;
> +
> + if (!idev->buf)
> + return IRQ_NONE;
> +
> + switch (status) {
> + case STATUS_M_START_SENT:
> + case STATUS_M_REPEATED_START_SENT:
> + ctrl = readb(idev->base + CORE_I2C_CTRL);
> + ctrl &= ~CTRL_STA;
> + writeb(idev->addr, idev->base + CORE_I2C_DATA);
> + writeb(ctrl, idev->base + CORE_I2C_CTRL);
> + if (idev->msg_len <= 0)
msg_len is unsigned, can't be < 0?
> + goto finished;
> + break;
> + case STATUS_M_ARB_LOST:
> + idev->msg_err = -EAGAIN;
> + goto finished;
> + case STATUS_M_SLAW_ACK:
> + case STATUS_M_TX_DATA_ACK:
> + if (idev->msg_len > 0)
> + mchp_corei2c_fill_tx(idev);
> + else
> + goto last_byte;
IMO this is a bit too much of goto here. Can't we have a flag for
last_byte and handle it at the end of the handler?
> + break;
> + case STATUS_M_TX_DATA_NACK:
> + case STATUS_M_SLAR_NACK:
> + case STATUS_M_SLAW_NACK:
> + idev->msg_err = -ENXIO;
> + goto last_byte;
> + case STATUS_M_SLAR_ACK:
> + ctrl = readb(idev->base + CORE_I2C_CTRL);
> + if (idev->msg_len == 1u) {
> + ctrl &= ~CTRL_AA;
> + writeb(ctrl, idev->base + CORE_I2C_CTRL);
> + } else {
> + ctrl |= CTRL_AA;
> + writeb(ctrl, idev->base + CORE_I2C_CTRL);
> + }
> + if (idev->msg_len < 1u)
> + goto last_byte;
> + break;
> + case STATUS_M_RX_DATA_ACKED:
> + mchp_corei2c_empty_rx(idev);
> + break;
> + case STATUS_M_RX_DATA_NACKED:
> + mchp_corei2c_empty_rx(idev);
> + if (idev->msg_len == 0)
> + goto last_byte;
> + break;
> + default:
> + break;
> + }
> +
> + return IRQ_HANDLED;
> +
> +last_byte:
> + /* On the last byte to be transmitted, send STOP */
> + mchp_corei2c_stop(idev);
> +finished:
> + complete(&idev->msg_complete);
> + return IRQ_HANDLED;
> +}
...
> +
> +static int mchp_corei2c_xfer_msg(struct mchp_corei2c_dev *idev,
> + struct i2c_msg *msg)
> +{
> + u8 ctrl;
> + unsigned long time_left;
> +
> + if (msg->len == 0)
> + return -EINVAL;
If your hardware cannot do zero length messages, you need to set up an
i2c_adapter_quirk struct with I2C_AQ_NO_ZERO_LEN.
> +
> + idev->addr = i2c_8bit_addr_from_msg(msg);
> + idev->msg_len = msg->len;
> + idev->buf = msg->buf;
> + idev->msg_err = 0;
> + idev->msg_read = (msg->flags & I2C_M_RD);
> +
> + reinit_completion(&idev->msg_complete);
> +
> + mchp_corei2c_core_enable(idev);
> +
> + ctrl = readb(idev->base + CORE_I2C_CTRL);
> + ctrl |= CTRL_STA;
> + writeb(ctrl, idev->base + CORE_I2C_CTRL);
> +
> + time_left = wait_for_completion_timeout(&idev->msg_complete,
> + MICROCHIP_I2C_TIMEOUT);
You should use adapter.timeout here, so it can be changed from
userspace.
> + if (!time_left)
> + return -ETIMEDOUT;
> +
> + return idev->msg_err;
> +}
> +
> +static int mchp_corei2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> + int num)
> +{
> + struct mchp_corei2c_dev *idev = i2c_get_adapdata(adap);
> + int i, ret;
> +
> + for (i = 0; i < num; i++) {
> + ret = mchp_corei2c_xfer_msg(idev, msgs++);
> + if (ret)
> + return ret;
> + }
> +
> + return num;
> +}
> +
> +static u32 mchp_corei2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
If you can't handle zero length messages, you need to mask
I2C_FUNC_SMBUS_QUICK out.
> +
> +static const struct i2c_algorithm mchp_corei2c_algo = {
> + .master_xfer = mchp_corei2c_xfer,
> + .functionality = mchp_corei2c_func,
> +};
> +
> +static int mchp_corei2c_probe(struct platform_device *pdev)
> +{
> + struct mchp_corei2c_dev *idev = NULL;
Unneeded initialization.
> + struct resource *res;
> + int irq, ret;
> + u32 val;
> +
> + idev = devm_kzalloc(&pdev->dev, sizeof(*idev), GFP_KERNEL);
> + if (!idev)
> + return -ENOMEM;
> +
> + idev->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> + if (IS_ERR(idev->base))
> + return PTR_ERR(idev->base);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return dev_err_probe(&pdev->dev, irq,
> + "missing interrupt resource\n");
> +
> + idev->i2c_clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(idev->i2c_clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(idev->i2c_clk),
> + "missing clock\n");
> +
> + idev->dev = &pdev->dev;
> + init_completion(&idev->msg_complete);
> + spin_lock_init(&idev->lock);
> +
> + val = device_property_read_u32(idev->dev, "clock-frequency",
> + &idev->bus_clk_rate);
This functions returns an int. So, 'ret' please which is also more
readable.
> + if (val) {
I think the missing check against 'division by zero' should go here.
> + dev_info(&pdev->dev, "default to 100kHz\n");
> + idev->bus_clk_rate = 100000;
> + }
> +
> + if (idev->bus_clk_rate > 400000)
> + return dev_err_probe(&pdev->dev, -EINVAL,
> + "clock-frequency too high: %d\n",
> + idev->bus_clk_rate);
> +
> + ret = devm_request_irq(&pdev->dev, irq, mchp_corei2c_isr, IRQF_SHARED,
> + pdev->name, idev);
Really SHARED?
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to claim irq %d\n", irq);
> +
> + ret = clk_prepare_enable(idev->i2c_clk);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to enable clock\n");
> +
> + ret = mchp_corei2c_init(idev);
> + if (ret) {
> + clk_disable_unprepare(idev->i2c_clk);
> + return dev_err_probe(&pdev->dev, ret, "failed to program clock divider\n");
> + }
> +
> + i2c_set_adapdata(&idev->adapter, idev);
> + snprintf(idev->adapter.name, sizeof(idev->adapter.name),
> + "Microchip I2C hw bus");
Most people add something like the base address here to distinguish
multiple instances.
> + idev->adapter.owner = THIS_MODULE;
> + idev->adapter.algo = &mchp_corei2c_algo;
> + idev->adapter.dev.parent = &pdev->dev;
> + idev->adapter.dev.of_node = pdev->dev.of_node;
> +
> + platform_set_drvdata(pdev, idev);
> +
> + ret = i2c_add_adapter(&idev->adapter);
> + if (ret) {
> + clk_disable_unprepare(idev->i2c_clk);
> + return ret;
> + }
> +
> + dev_info(&pdev->dev, "Microchip I2C Probe Complete\n");
> +
> + return 0;
> +}
Rest looks good, Thank you for the submission.
All the best,
Wolfram
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists