| 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: Mon, 3 Apr 2023 15:16:22 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Jiawen Wu <jiawenwu@...stnetic.com>
Cc: netdev@...r.kernel.org, linux@...linux.org.uk,
mengyuanlou@...-swift.com
Subject: Re: [PATCH net-next 4/6] net: txgbe: Support GPIO to SFP socket
Please Cc: The GPIO mailing list, they know more about GPIO drivers
than the netdev people.
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> index eb89a274083e..dff0d573ee33 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> @@ -1348,7 +1348,8 @@ void wx_free_irq(struct wx *wx)
> free_irq(entry->vector, q_vector);
> }
>
> - free_irq(wx->msix_entries[vector].vector, wx);
> + if (wx->mac.type == wx_mac_em)
> + free_irq(wx->msix_entries[vector].vector, wx);
> }
> EXPORT_SYMBOL(wx_free_irq);
>
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> index 072aa2bd3fdc..89bba827edf2 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> @@ -79,7 +79,9 @@
> #define WX_GPIO_INTMASK 0x14834
> #define WX_GPIO_INTTYPE_LEVEL 0x14838
> #define WX_GPIO_POLARITY 0x1483C
> +#define WX_GPIO_INTSTATUS 0x14844
> #define WX_GPIO_EOI 0x1484C
> +#define WX_GPIO_EXT 0x14850
>
> /*********************** Transmit DMA registers **************************/
> /* transmit global control */
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_hw.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_hw.c
> index ebc46f3be056..b87034e57140 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_hw.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_hw.c
> @@ -256,6 +256,7 @@ int txgbe_validate_eeprom_checksum(struct wx *wx, u16 *checksum_val)
> static void txgbe_reset_misc(struct wx *wx)
> {
> wx_reset_misc(wx);
> + wr32(wx, WX_GPIO_DDR, 0x32);
No magic numbers, use #define's.
DDR is Data Direction Register? So are you configuring some of the
pins as outputs? Is that needed? The SFP driver will request how it
wants the pins configuring, so there should not be any need to hard
code it here.
> txgbe_init_thermal_sensor_thresh(wx);
> }
>
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> index 319d56720c06..caaefc20afb9 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> @@ -82,6 +82,10 @@ static int txgbe_enumerate_functions(struct wx *wx)
> **/
> static void txgbe_irq_enable(struct wx *wx, bool queues)
> {
> + wr32(wx, WX_GPIO_INTEN, TXGBE_GPIOBIT_0 | TXGBE_GPIOBIT_2 | TXGBE_GPIOBIT_3);
> + wr32(wx, WX_GPIO_INTTYPE_LEVEL, TXGBE_GPIOBIT_0 | TXGBE_GPIOBIT_2 | TXGBE_GPIOBIT_3);
Again, the SFP code will ask that interrupts are enabled for the GPIOs
it is interested in. There should not be any need to hard code
this. Disable all interrupts until they are requested.
> + wr32(wx, WX_PX_MISC_IEN, TXGBE_PX_MISC_IEN_MASK);
> +
> /* unmask interrupt */
> wx_intr_enable(wx, TXGBE_INTR_MISC(wx));
> if (queues)
> @@ -129,17 +133,6 @@ static irqreturn_t txgbe_intr(int __always_unused irq, void *data)
> return IRQ_HANDLED;
> }
>
> +static int txgbe_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct wx *wx = gpiochip_get_data(chip);
> + int val, dir;
> +
> + dir = chip->get_direction(chip, offset);
> + if (dir == GPIO_LINE_DIRECTION_IN)
> + val = rd32m(wx, WX_GPIO_EXT, BIT(offset));
> + else
> + val = rd32m(wx, WX_GPIO_DR, BIT(offset));
> +
> + return !!(val & BIT(offset));
> +}
> +
> +static int txgbe_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct wx *wx = gpiochip_get_data(chip);
> + u32 val;
> +
> + val = rd32(wx, WX_GPIO_DDR);
> + if (BIT(offset) & val)
> + return GPIO_LINE_DIRECTION_OUT;
> +
> + return GPIO_LINE_DIRECTION_IN;
> +}
> +
> +static int txgbe_gpio_direction_in(struct gpio_chip *chip, unsigned int offset)
> +{
> + return 0;
> +}
This is where the write to DDR should probably happen?
> +
> +static int txgbe_gpio_direction_out(struct gpio_chip *chip, unsigned int offset,
> + int val)
> +{
> + struct wx *wx = gpiochip_get_data(chip);
> + u32 mask;
> + int dir;
> +
> + dir = chip->get_direction(chip, offset);
> + if (dir == GPIO_LINE_DIRECTION_IN)
> + return 0;
Don't assume in by default.
> + mask = BIT(offset) | BIT(offset - 1);
> + if (val)
> + wr32m(wx, WX_GPIO_DR, mask, mask);
> + else
> + wr32m(wx, WX_GPIO_DR, mask, 0);
> +
> + return 0;
> +}
> +
> +static void txgbe_gpio_irq_ack(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + irq_hw_number_t hwirq = irqd_to_hwirq(d);
> + struct wx *wx = gpiochip_get_data(gc);
> +
> + wr32(wx, WX_GPIO_EOI, BIT(hwirq));
> +}
> +
> +static void txgbe_gpio_irq_mask(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + irq_hw_number_t hwirq = irqd_to_hwirq(d);
> + struct wx *wx = gpiochip_get_data(gc);
> +
> + gpiochip_disable_irq(gc, hwirq);
> +
> + wr32m(wx, WX_GPIO_INTMASK, BIT(hwirq), BIT(hwirq));
> +}
> +
> +static void txgbe_gpio_irq_unmask(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + irq_hw_number_t hwirq = irqd_to_hwirq(d);
> + struct wx *wx = gpiochip_get_data(gc);
> +
> + gpiochip_enable_irq(gc, hwirq);
> +
> + wr32m(wx, WX_GPIO_INTMASK, BIT(hwirq), 0);
> +}
> +
> +static int txgbe_gpio_set_type(struct irq_data *d, unsigned int type)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + irq_hw_number_t hwirq = irqd_to_hwirq(d);
> + struct wx *wx = gpiochip_get_data(gc);
> + u32 level, polarity;
> +
> + level = rd32(wx, WX_GPIO_INTTYPE_LEVEL);
> + polarity = rd32(wx, WX_GPIO_POLARITY);
> +
> + 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);
> + break;
> + case IRQ_TYPE_LEVEL_HIGH:
> + level &= ~BIT(hwirq);
> + polarity |= BIT(hwirq);
> + break;
> + case IRQ_TYPE_LEVEL_LOW:
> + level &= ~BIT(hwirq);
> + polarity &= ~BIT(hwirq);
> + break;
> + }
You have two bits, level and priority, so 4 states. But you have
handling 5 different types? Please return -EOPNOTSUPP for whatever you
don't support.
> +
> + if (type & IRQ_TYPE_LEVEL_MASK)
> + irq_set_handler_locked(d, handle_level_irq);
> + else if (type & IRQ_TYPE_EDGE_BOTH)
> + irq_set_handler_locked(d, handle_edge_irq);
I think this is part of the answer to my question above. Please
re-write this code to make it more obvious.
Andrew
Powered by blists - more mailing lists