Prev: [PATCH 06/37] x86: call early_res_to_bootmem one time
Next: lockdep: Add information of file and line to lockdep_map
From: David Miller on 16 Jan 2010 04:30
From: "Ha, Tristram" <Tristram.Ha(a)Micrel.Com>
Date: Fri, 15 Jan 2010 18:57:59 -0800

> The KSZ8842 has a switch with lots of hardware configurations. The =
> driver uses the proc system to allow users to configure the switch. If =
> this is not desired the whole thing can be removed by not calling the =
> init_proc() function.

I think there needs to be a serious discussion about how
this driver uses bridge layer internals by doing things like:

+/* Needed for STP support. */
+#ifdef CONFIG_KSZ8842_STP
+#include <../net/bridge/br_private.h>
+#endif

and uses procfs to configure the ports.

Stephen please look this over and make suggestions for better
ways to support and configure these kinds of devices.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alan Cox on 16 Jan 2010 09:50
> +/*
> + * PCI Configuration Space Registers
> + */

We have existing standard defines for most of these that should be used
instead where relevant. The code appears not use most of these defines
for generic PCI stuff anyway.


> +#define PORT_CTRL_ADDR(port, addr) \
> + (addr = KS8842_PORT_1_CTRL_1 + port * \
> + (KS8842_PORT_2_CTRL_1 - KS8842_PORT_1_CTRL_1))

Brackets around port so that things like PORT_CTRL_ADDR(x + 1) work
as expected


> +/*
> + * Hardware register access macros
> + */
> +
> +#define hw_dis_intr_sync(hw) hw_dis_intr(hw)
> +
> +#define HW_R8(hw, addr, data) (*(data) = readb((hw)->ioaddr + (addr)))
> +#define HW_W8(hw, addr, data) writeb((data), (hw)->ioaddr + (addr))
> +
> +#define HW_R16(hw, addr, data) (*(data) = readw((hw)->ioaddr + (addr)))
> +#define HW_W16(hw, addr, data) writew((data), (hw)->ioaddr + (addr))
> +
> +#define HW_R32(hw, addr, data) (*(data) = readl((hw)->ioaddr + (addr)))
> +#define HW_W32(hw, addr, data) writel((data), (hw)->ioaddr + (addr))
> +
> +#define HW_PCI_READ_BYTE(hw, addr, data) \
> + pci_read_config_byte((struct pci_dev *) (hw)->pdev, addr, data)
> +
> +#define HW_PCI_READ_WORD(hw, addr, data) \
> + pci_read_config_word((struct pci_dev *) (hw)->pdev, addr, data)
> +
> +#define HW_PCI_READ_DWORD(hw, addr, data) \
> + pci_read_config_dword((struct pci_dev *) (hw)->pdev, addr, data)
> +
> +#define HW_PCI_WRITE_BYTE(hw, addr, data) \
> + pci_write_config_byte((struct pci_dev *) (hw)->pdev, addr, data)
> +
> +#define HW_PCI_WRITE_WORD(hw, addr, data) \
> + pci_write_config_word((struct pci_dev *) (hw)->pdev, addr, data)
> +
> +#define HW_PCI_WRITE_DWORD(hw, addr, data) \
> + pci_write_config_dword((struct pci_dev *) (hw)->pdev, addr, data)

This sort of stuff just obfuscates things so the defines ought to get
removed so the code is more readable to all (remember the Linux kernel
code needs to be generally readable not just readable by Micrel people)


> + * delay_milli - delay in millisecond
> + * @millisec: Number of milliseconds to delay.
> + *
> + * This routine delays in milliseconds.
> + */
> +static void delay_milli(uint millisec)
> +{
> + unsigned long ticks = millisec * HZ / 1000;
> +
> + if (!ticks || in_interrupt())
> + mdelay(millisec);
> + else {
> + set_current_state(TASK_INTERRUPTIBLE);
> + schedule_timeout(ticks);

msleep() is probably what you want for this part ? Note that you really
don't want to be spinning for milliseconds in the IRQ paths if at all
possible.

The DEBUG ioctl is a bit odd - you define it and it does nothing - just a
left over ?



> +
> +#define PCI_VENDOR_ID_KS884X 0x16C6
> +#define PCI_DEVICE_ID_KS8841 0x8841
> +#define PCI_DEVICE_ID_KS8842 0x8842

Those belong in the pci device id header.


In your pci init function you do the following

> + hw->pdev = pdev;


If you make a private copy of pdev in your struct you should refcount it
and use pci_dev_get/pci_dev_put when you take and release the reference.


The proc stuff probably belongs in sysfs nowdays


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Michał Mirosław on 16 Jan 2010 11:10
Hello,

2010/1/16 Ha, Tristram <Tristram.Ha(a)micrel.com>:
> This is a new network driver for Micrel KSZ8841/KSZ8842 PCI Ethernet chips. �The same driver can run both chips at the same time. �It supports IPv4 TCP hardware checksumming and so can use scatter/gather transmission.
>
> The KSZ8842 has a switch with lots of hardware configurations. �The driver uses the proc system to allow users to configure the switch. �If this is not desired the whole thing can be removed by not calling the init_proc() function.
>
> The KSZ8842 switch has 2 ports. �Some users like to take direct control of those ports. �So KSZ8842 has a multiple devices mode in which the driver creates another network device so that users can specify which port to send packets. �This mode is enabled by passing the "multi_dev=1" parameter to the driver during loading.
[...]

I briefly looked over the datasheet for your device and it looks like
it has the same basic design idea as a "ROBO" switch from Broadcom
that is used in bcm53xx SoCs. This can be seen as a 3-port switch with
one port fixed to be link to CPU (a special kind of PHY).

If my assumptions are correct, then I thing it would be better to have
a driver that presents this device as two: one - an ethernet device,
and second - a switch management interface like the one used in
OpenWRT.

Here are some links for reference:
http://downloads.openwrt.org/kamikaze/8.09.2/kamikaze_8.09.2_source.tar.bz2;
subdir: package/switch/src/
-> switch management driver for ADMTEK Adm6996 and Broadcom BCM5325E/536x

(I'm adding switch driver's authors to CC list. I hope I picket the
correct address for Felix from 3 in the sources. :-)

Best Regards,
Micha� Miros�aw
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Felix Fietkau on 16 Jan 2010 11:40
On 2010-01-16 5:00 PM, Micha� Miros�aw wrote:
> Hello,
>
> 2010/1/16 Ha, Tristram <Tristram.Ha(a)micrel.com>:
>> This is a new network driver for Micrel KSZ8841/KSZ8842 PCI Ethernet chips. The same driver can run both chips at the same time. It supports IPv4 TCP hardware checksumming and so can use scatter/gather transmission.
>>
>> The KSZ8842 has a switch with lots of hardware configurations. The driver uses the proc system to allow users to configure the switch. If this is not desired the whole thing can be removed by not calling the init_proc() function.
>>
>> The KSZ8842 switch has 2 ports. Some users like to take direct control of those ports. So KSZ8842 has a multiple devices mode in which the driver creates another network device so that users can specify which port to send packets. This mode is enabled by passing the "multi_dev=1" parameter to the driver during loading.
> [...]
>
> I briefly looked over the datasheet for your device and it looks like
> it has the same basic design idea as a "ROBO" switch from Broadcom
> that is used in bcm53xx SoCs. This can be seen as a 3-port switch with
> one port fixed to be link to CPU (a special kind of PHY).
>
> If my assumptions are correct, then I thing it would be better to have
> a driver that presents this device as two: one - an ethernet device,
> and second - a switch management interface like the one used in
> OpenWRT.
>
> Here are some links for reference:
> http://downloads.openwrt.org/kamikaze/8.09.2/kamikaze_8.09.2_source.tar.bz2;
> subdir: package/switch/src/
> -> switch management driver for ADMTEK Adm6996 and Broadcom BCM5325E/536x
Don't look at that ;)
The code you're referring to is just a simple hack of mine that is a
few years old now. It's going to be removed as soon as the drivers have
been ported to a proper API.

I've been working on a switch configuration API based on netlink, which
is being used on some of the other platforms in OpenWrt.

You can find the sources here:
https://dev.openwrt.org/export/19173/trunk/target/linux/generic-2.6/files/drivers/net/phy/swconfig.c
https://dev.openwrt.org/export/19173/trunk/target/linux/generic-2.6/files/include/linux/switch.h

It's meant to be used with drivers that hook into the PHY abstraction layer.
You can find an example of such a driver here:
https://dev.openwrt.org/export/19173/trunk/target/linux/generic-2.6/files/drivers/net/phy/rtl8306.c

I'll do the final cleanups and submit it for review once I have time to do so.

- Felix
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Ha, Tristram on 19 Jan 2010 15:10
David Miller wrote:
> From: "Ha, Tristram" <Tristram.Ha(a)Micrel.Com>
> Date: Fri, 15 Jan 2010 18:57:59 -0800
>
>> The KSZ8842 has a switch with lots of hardware configurations. The =
>> driver uses the proc system to allow users to configure the switch.
>> If = this is not desired the whole thing can be removed by not
calling the =
>> init_proc() function.
>
> I think there needs to be a serious discussion about how this driver
uses bridge layer internals
> by doing things like:
>
> +/* Needed for STP support. */
> +#ifdef CONFIG_KSZ8842_STP
> +#include <../net/bridge/br_private.h>
> +#endif
>
> and uses procfs to configure the ports.
>
> Stephen please look this over and make suggestions for better ways to
support and configure
> these kinds of devices.
>
> Thanks.

I like to explain a little bit about this Spanning Tree Protocol
support.

Micrel KSZ8842's 3-port switch and Micrel's other 5-port switches have
port controls to enable/disable tx and rx and stop MAC address learning.
They are supposed to help run STP more efficiently, but somebody needs
to control those ports.

From my observation of how the brctl application controls the network
devices when running STP, I know the kernel bridge puts all the devices
under it in promiscuous mode and declares the state of each bridge port
associated with the device blocked or forwarding depending on the BPDU
frames received. When the port is blocked, the device is still active
and passes all frames to the host. The bridge only looks at BPDU frames
and drops all other frames. It is better to just shut off the port.

From the time when the KSZ8842 driver was developed for the Linux 2.4
kernel I looked for a kernel API to tell the bridge port's state so the
device driver can shut off the port if necessary. I could not find one
and so I came up with this hack to look at the bridge port's structure
directly. It looks dangerous but is quite safe. The driver only looks
at the bridge port state variable and finds out the MAC address
associated with the bridge device. It can get the state definitions
from the if_bridge header. The private bridge structure may change in
the future and break the code, but as kernel network interfaces are
changing all the time, the driver just needs to be modified for the new
version. To avoid this situation, the kernel may need to export two
functions to tell the bridge port state and bridge device address and
put the prototypes in the if_bridge header.

Now for the driver implementation for STP support. I programmed the
switch's static MAC table to always pass the following frames to the
host: BPDU frames with specific multicast address, broadcast frames,
unicast frames with the device bridge's MAC address, and multicast
frames with ICMPv6 multicast address. All other frames are not passed
to the host and are handled by the switch, forwarding each frame with
its standard forwarding logic. The port can be shut off if it is
blocked and those frames will not pass through that port. The host gets
BPDU frames so that the bridge can determine each port's state. The
other broadcast, unicast, and multicast frames passed to the host are
necessary if some other network devices want to communicate with the
host. As the forwarding is done by hardware rather than software,
overall performance does increase.

I did verify the driver disables or enables the port appropriately when
the bridge port state changes, but as I do not have the experience of
running a full-scale Spanning Tree, I do not know if this hardware
implementation behaves the same as the software one provided by the
kernel and brctl. I also did not try using VLAN.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2 3 4
Prev: [PATCH 06/37] x86: call early_res_to_bootmem one time
Next: lockdep: Add information of file and line to lockdep_map