| 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: <1317d02a-aaf4-943d-37f7-90f11b237f72@csgroup.eu>
Date: Sun, 21 Aug 2022 07:24:18 +0000
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Arminder Singh <arminders208@...look.com>
CC: Sven Peter <sven@...npeter.dev>, Hector Martin <marcan@...can.st>,
Paul Mackerras <paulus@...ba.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
Alyssa Rosenzweig <alyssa@...enzweig.io>,
"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon
Le 20/08/2022 à 21:45, Arminder Singh a écrit :
> This is the first time I'm interacting with the Linux mailing lists, so
> please don't eviscerate me *too much* if I get the formatting wrong.
> Of course I'm always willing to take criticism and improve my formatting
> in the future.
The above text should go after you signed-off-by, after the --- , so
that it get's ignored by 'git am' when applying.
>
> This patch adds support for IRQs to the PASemi I2C controller driver.
> This will allow for faster performing I2C transactions on Apple Silicon
> hardware, as previously, the driver was forced to poll the SMSTA register
> for a set amount of time.
>
> With this patchset the driver on Apple silicon hardware will instead wait
> for an interrupt which will signal the completion of the I2C transaction.
> The timeout value for this completion will be the same as the current
> amount of time the I2C driver polls for.
>
> This will result in some performance improvement since the driver will be
> waiting for less time than it does right now on Apple Silicon hardware.
>
> The patch right now will only enable IRQs for Apple Silicon I2C chips,
> and only if it's able to successfully request the IRQ from the kernel.
>
> === Testing ===
>
> This patch has been tested on both the mainline Linux kernel tree and
> the Asahi branch (https://github.com/AsahiLinux/linux.git) on both an
> M1 and M2 MacBook Air, and it compiles successfully as both a module and
> built-in to the kernel itself. The patch in both trees successfully boots
> to userspace without any hitch.
>
> I do not have PASemi hardware on hand unfortunately, so I'm unable to test
> the impact of this patch on old PASemi hardware. This is also why I've
> elected to do the IRQ request and enablement on the Apple platform driver
> and not in the common file, as I'm not sure if PASemi hardware supports
> IRQs.
>
> I also fixed a quick checkpatch warning on line 303. "i ++" is now "i++".
>
> Any and all critiques of the patch would be well appreciated.
That as well shouldn't be part of the commit message.
>
>
>
>
> Signed-off-by: Arminder Singh <arminders208@...look.com>
> ---
> drivers/i2c/busses/i2c-pasemi-core.c | 29 ++++++++++++++++++++----
> drivers/i2c/busses/i2c-pasemi-core.h | 5 ++++
> drivers/i2c/busses/i2c-pasemi-platform.c | 8 +++++++
> 3 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-pasemi-core.c b/drivers/i2c/busses/i2c-pasemi-core.c
> index 9028ffb58cc0..375aa9528233 100644
> --- a/drivers/i2c/busses/i2c-pasemi-core.c
> +++ b/drivers/i2c/busses/i2c-pasemi-core.c
> @@ -21,6 +21,7 @@
> #define REG_MTXFIFO 0x00
> #define REG_MRXFIFO 0x04
> #define REG_SMSTA 0x14
> +#define REG_IMASK 0x18
> #define REG_CTL 0x1c
> #define REG_REV 0x28
>
> @@ -80,14 +81,21 @@ static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
> {
> int timeout = 10;
> unsigned int status;
> + unsigned int bitmask = SMSTA_XEN | SMSTA_MTN;
Used only inside the if (smbus->use_irq), bitmask should be defined only
in that scope.
>
> - status = reg_read(smbus, REG_SMSTA);
This should go in the else block.
> -
> - while (!(status & SMSTA_XEN) && timeout--) {
> - msleep(1);
> + if (smbus->use_irq) {
> + reinit_completion(&smbus->irq_completion);
> + reg_write(smbus, REG_IMASK, bitmask);
> + wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(10));
> status = reg_read(smbus, REG_SMSTA);
> + } else {
status is uninitialised when entering the while loop.
> + while (!(status & SMSTA_XEN) && timeout--) {
> + msleep(1);
> + status = reg_read(smbus, REG_SMSTA);
> + }
> }
>
> +
Why a second blank line here ?
> /* Got NACK? */
> if (status & SMSTA_MTN)
> return -ENXIO;
> @@ -300,7 +308,7 @@ static int pasemi_smb_xfer(struct i2c_adapter *adapter,
> case I2C_SMBUS_BLOCK_DATA:
> case I2C_SMBUS_BLOCK_PROC_CALL:
> data->block[0] = len;
> - for (i = 1; i <= len; i ++) {
> + for (i = 1; i <= len; i++) {
This shouldn't be part of this patch, it is unrelated.
> rd = RXFIFO_RD(smbus);
> if (rd & MRXFIFO_EMPTY) {
> err = -ENODATA;
> @@ -348,6 +356,8 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus)
> if (smbus->hw_rev != PASEMI_HW_REV_PCI)
> smbus->hw_rev = reg_read(smbus, REG_REV);
>
> + reg_write(smbus, REG_IMASK, 0);
> +
> pasemi_reset(smbus);
>
> error = devm_i2c_add_adapter(smbus->dev, &smbus->adapter);
> @@ -356,3 +366,12 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus)
>
> return 0;
> }
> +
> +irqreturn_t pasemi_irq_handler(int irq, void *dev_id)
> +{
> + struct pasemi_smbus *smbus = (struct pasemi_smbus *)dev_id;
dev_id is void*, the cast is not needed.
> +
> + reg_write(smbus, REG_IMASK, 0);
> + complete(&smbus->irq_completion);
> + return IRQ_HANDLED;
> +}
> diff --git a/drivers/i2c/busses/i2c-pasemi-core.h b/drivers/i2c/busses/i2c-pasemi-core.h
> index 4655124a37f3..045e4a9a3d13 100644
> --- a/drivers/i2c/busses/i2c-pasemi-core.h
> +++ b/drivers/i2c/busses/i2c-pasemi-core.h
> @@ -7,6 +7,7 @@
> #include <linux/i2c-smbus.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
> +#include <linux/completion.h>
>
> #define PASEMI_HW_REV_PCI -1
>
> @@ -16,6 +17,10 @@ struct pasemi_smbus {
> void __iomem *ioaddr;
> unsigned int clk_div;
> int hw_rev;
> + int use_irq;
> + struct completion irq_completion;
Keep same alignment for member name.
> };
>
> int pasemi_i2c_common_probe(struct pasemi_smbus *smbus);
> +
> +irqreturn_t pasemi_irq_handler(int irq, void *dev_id);
> diff --git a/drivers/i2c/busses/i2c-pasemi-platform.c b/drivers/i2c/busses/i2c-pasemi-platform.c
> index 88a54aaf7e3c..ee1c84e7734b 100644
> --- a/drivers/i2c/busses/i2c-pasemi-platform.c
> +++ b/drivers/i2c/busses/i2c-pasemi-platform.c
> @@ -49,6 +49,7 @@ static int pasemi_platform_i2c_probe(struct platform_device *pdev)
> struct pasemi_smbus *smbus;
> u32 frequency;
> int error;
> + int irq_num;
>
> data = devm_kzalloc(dev, sizeof(struct pasemi_platform_i2c_data),
> GFP_KERNEL);
> @@ -82,6 +83,13 @@ static int pasemi_platform_i2c_probe(struct platform_device *pdev)
> if (error)
> goto out_clk_disable;
>
> + smbus->use_irq = 0;
> + init_completion(&smbus->irq_completion);
> + irq_num = platform_get_irq(pdev, 0);
> + error = request_irq(irq_num, pasemi_irq_handler, 0, "pasemi_apple_i2c", (void *)smbus);
> +
> + if (!error)
> + smbus->use_irq = 1;
> platform_set_drvdata(pdev, data);
>
> return 0;
> --
> 2.34.1
>
Powered by blists - more mailing lists