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: <ZF1T62BnVFgR33w0@surfacebook> Date: Thu, 11 May 2023 23:45:31 +0300 From: andy.shevchenko@...il.com To: Jiawen Wu <jiawenwu@...stnetic.com> Cc: netdev@...r.kernel.org, jarkko.nikula@...ux.intel.com, andriy.shevchenko@...ux.intel.com, mika.westerberg@...ux.intel.com, jsd@...ihalf.com, Jose.Abreu@...opsys.com, andrew@...n.ch, hkallweit1@...il.com, linux@...linux.org.uk, linux-i2c@...r.kernel.org, linux-gpio@...r.kernel.org, mengyuanlou@...-swift.com Subject: Re: [PATCH net-next v7 6/9] net: txgbe: Support GPIO to SFP socket Tue, May 09, 2023 at 10:27:31AM +0800, Jiawen Wu kirjoitti: > Register GPIO chip and handle GPIO IRQ for SFP socket. ... > +#include <linux/gpio/consumer.h> What for? > +#include <linux/gpio/machine.h> > +#include <linux/gpio/driver.h> ... > +static int txgbe_gpio_get(struct gpio_chip *chip, unsigned int offset) > +{ > + struct wx *wx = gpiochip_get_data(chip); > + struct txgbe *txgbe = wx->priv; > + int val; > + > + val = rd32m(wx, WX_GPIO_EXT, BIT(offset)); > + > + txgbe->gpio_orig &= ~BIT(offset); > + txgbe->gpio_orig |= val; This can use standard pattern in conjunction with simple rd32() call: txgbe->gpio_orig = (txgbe->gpio_orig & ~BIT(offset)) | (val & BIT(offset)); otherwise it's not immediately obvious that val can have only one bit set. > + return !!(val & BIT(offset)); > +} ... > +static int txgbe_gpio_direction_out(struct gpio_chip *chip, unsigned int offset, > + int val) > +{ > + struct wx *wx = gpiochip_get_data(chip); > + u32 mask; > + > + mask = BIT(offset) | BIT(offset - 1); > + if (val) > + wr32m(wx, WX_GPIO_DR, mask, mask); > + else > + wr32m(wx, WX_GPIO_DR, mask, 0); > + > + wr32m(wx, WX_GPIO_DDR, BIT(offset), BIT(offset)); Can you explain, what prevents to have this flow to be interleaved by other API calls, like ->direction_in()? Didn't you missed proper locking schema? > + return 0; > +} ... > + switch (type) { > + case IRQ_TYPE_EDGE_BOTH: > + level |= BIT(hwirq); > + break; > + case IRQ_TYPE_EDGE_RISING: > + level |= BIT(hwirq); > + polarity |= BIT(hwirq); > + break; > + case IRQ_TYPE_EDGE_FALLING: > + level |= BIT(hwirq); > + polarity &= ~BIT(hwirq); This... > + break; > + case IRQ_TYPE_LEVEL_HIGH: > + level &= ~BIT(hwirq); ...and this can be done outside of the switch-case. Then you simply set certain bits where it's needed. > + polarity |= BIT(hwirq); > + break; > + case IRQ_TYPE_LEVEL_LOW: > + level &= ~BIT(hwirq); > + polarity &= ~BIT(hwirq); > + break; default? > + } ... > + /* workaround for hysteretic gpio interrupts */ GPIO ... > + gc->can_sleep = false; Useless, kzalloc() already sets this to 0. ... > + girq->num_parents = 1; > + girq->parents = devm_kcalloc(&pdev->dev, 1, sizeof(*girq->parents), Use girq->num_parents instead of explicit 1 in this call. > + GFP_KERNEL); Also with struct device *dev = &pdev->dev; this (and others) can be modified as girq->parents = devm_kcalloc(dev, girq->num_parents, sizeof(*girq->parents), > + if (!girq->parents) > + return -ENOMEM; ... > +#define TXGBE_PX_MISC_IEN_MASK ( \ > + TXGBE_PX_MISC_ETH_LKDN | \ > + TXGBE_PX_MISC_DEV_RST | \ > + TXGBE_PX_MISC_ETH_EVENT | \ > + TXGBE_PX_MISC_ETH_LK | \ > + TXGBE_PX_MISC_ETH_AN | \ > + TXGBE_PX_MISC_INT_ERR | \ > + TXGBE_PX_MISC_GPIO) Wouldn't be better #define TXGBE_PX_MISC_IEN_MASK \ (TXGBE_PX_MISC_ETH_LKDN | TXGBE_PX_MISC_ETH_LK | \ TXGBE_PX_MISC_ETH_EVENT | TXGBE_PX_MISC_ETH_AN | \ TXGBE_PX_MISC_DEV_RST | TXGBE_PX_MISC_INT_ERR | \ TXGBE_PX_MISC_GPIO) ? -- With Best Regards, Andy Shevchenko
Powered by blists - more mailing lists