From: Richard Dawe <hidden> Date: 2005-02-26 17:11:02
Hello.
The attached patch against 2.6.11-rc5 adds support for setting the
message level, which controls which messages are actually printk()'d.
Changes:
* A module option "debug". This option is a bitfield of message options,
like 8139cp's "debug" parameter.
* Support "ethtool -s ... msglvl". This sets the bitfield.
* Various debug messages are now enabled on NETIF_MSG_HW and are printed
using KERN_DEBUG.
* Messages are printed when the interface goes up or down.
There seems to be a mixture of drivers using a bitfield and a level.
Which is the currently preferred mechanism?
Bye, Rich =]
Signed-Off-By: Richard Dawe <redacted>
@@ -79,12 +79,14 @@ VERSION 2.2LK <2005/01/25>printk("Assertion failed! %s,%s,%s,line=%d\n",\#expr,__FILE__,__FUNCTION__,__LINE__); \}-#define dprintk(fmt, args...) do { printk(PFX fmt, ## args); } while (0)#else#define assert(expr) do {} while (0)-#define dprintk(fmt, args...) do {} while (0)#endif /* RTL8169_DEBUG */+#define DPRINTK(nlevel, klevel, fmt, args...) \+(void)((NETIF_MSG_##nlevel&tp->msg_enable)&&\+printk(KERN_##klevelPFX"%s: "fmt,dev->name,##args))+
1 - I am not fond of shouting macro. Everything starts turnings caps.
Any reason to not use "dprintk" ?
2 - Imho the driver should not poke its nose into the guts of netif_msg_xxx().
It defeats its whole purpose. Any objection to not use it explicitely ?
3 - If PFX is included, we'll have a mix of printk and dprintk. My personal
taste would be to not include it in the definition of the macro.
Variable declaration and code are always separated by an empty
line in the current driver. Please keep it this way.
for (p = mac_print; p->msg; p++) {
if (tp->mac_version == p->version) {
- dprintk("mac_version == %s (%04d)\n", p->msg,
- p->version);
+ if (netif_msg_hw(tp))
+ printk(KERN_DEBUG
+ "mac_version == %s (%04d)\n",
No need to add a line: you are allowed to use the whole 80 cols range.
[...]
quoted hunk
@@ -1091,7 +1122,7 @@ static void rtl8169_phy_timer(unsigned l if (tp->link_ok(ioaddr)) goto out_unlock;- printk(KERN_WARNING PFX "%s: PHY reset until link up\n", dev->name);+ DPRINTK(LINK, WARNING, "PHY reset until link up\n"); tp->phy_reset_enable(ioaddr);
@@ -1169,7 +1200,8 @@ rtl8169_init_board(struct pci_dev *pdev, /* dev zeroed in alloc_etherdev */ dev = alloc_etherdev(sizeof (*tp)); if (dev == NULL) {- printk(KERN_ERR PFX "unable to alloc new ethernet\n");+ if (debug & NETIF_MSG_PROBE)+ printk(KERN_ERR PFX "unable to alloc new ethernet\n"); goto err_out; }
Can you do something like:
struct {
u32 msg_enable;
} debug;
This way it will be possible to issue netif_msg_probe(&debug).
From: Jeff Garzik <hidden> Date: 2005-02-26 21:20:43
Francois Romieu wrote:
Jeff, can you send a ack/nack if you disagree with the remarks below ?
Richard Dawe [off-list ref] :
[...]
quoted
There seems to be a mixture of drivers using a bitfield and a level.
Which is the currently preferred mechanism?
They do not offer exactly the same range. I prefer to keep both as the
module option is not that expensive.
* The preferred mechanism is to have an integer verbosity level 'debug',
which is converted using netif_msg_init() into a bitmap.
* PFX should only be used in probe paths. In all other cases, dev->name
should be used.
* I strongly agree with the comment "Imho the driver should not poke its
nose into the guts of netif_msg_xxx()"
Jeff
From: Richard Dawe <hidden> Date: 2005-02-27 22:43:01
Hello.
Thanks for reviewing the patch, Francois and Jeff. I'll send an updated
version sometime in the next week.
Francois Romieu wrote:
Jeff, can you send a ack/nack if you disagree with the remarks below ?
Richard Dawe [off-list ref] :
[...]
quoted
There seems to be a mixture of drivers using a bitfield and a level.
Which is the currently preferred mechanism?
They do not offer exactly the same range. I prefer to keep both as the
module option is not that expensive.
OK.
[snip]
1 - I am not fond of shouting macro. Everything starts turnings caps.
Any reason to not use "dprintk" ?
(dprintk vs. DPRINTK)
I prefer macros to be uppercase, to make it obvious that they're macros.
But this isn't a strong preference.
I'll make it lowercase.
2 - Imho the driver should not poke its nose into the guts of netif_msg_xxx().
It defeats its whole purpose. Any objection to not use it explicitely ?
No objection at all.
In my first patch I did exactly that. But then I used the e100 driver as
a model, which sticks its nose into the guts.
I'll use the netif_msg_xxx() macros.
3 - If PFX is included, we'll have a mix of printk and dprintk. My personal
taste would be to not include it in the definition of the macro.
I'll go with Jeff here, which is that "PFX should only be used in probe
paths".
[snip]
It's up to you but I'd rather see:
#define RTL8169_DEF_MSG_ENABLE \
(NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_LINK)
/* dev zeroed in alloc_etherdev */
dev = alloc_etherdev(sizeof (*tp));
if (dev == NULL) {
- printk(KERN_ERR PFX "unable to alloc new ethernet\n");
+ if (debug & NETIF_MSG_PROBE)
+ printk(KERN_ERR PFX "unable to alloc new ethernet\n");
goto err_out;
}
Can you do something like:
struct {
u32 msg_enable;
} debug;
This way it will be possible to issue netif_msg_probe(&debug).
Original dprintk or the DPRINTK used in my patch? If you mean DPRINTK,
then it wouldn't work, because DPRINTK includes dev->name. At this point
in the code, dev->name is not defined.
Perhaps I could modifying DPRINTK (*) to use dev->name if defined,
otherwise fall back on PFX.
(*) I'm not ignoring the future renaming of DPRINTK to dprintk. I'm just
trying to avoid confusion when talking about this patch.
Thanks, bye, Rich =]
--
Richard Dawe [ http://homepages.nildram.co.uk/~phekda/richdawe/ ]
"You can't evaluate a man by logic alone."
-- McCoy, "I, Mudd", Star Trek
3 - If PFX is included, we'll have a mix of printk and dprintk. My personal
taste would be to not include it in the definition of the macro.
I'll go with Jeff here, which is that "PFX should only be used in probe
paths".
Fine.
[...]
I think I did that for consistency with another printk that was split
across lines.
They were split when they could not fit on a single line. OTOH I did not
hunt them when they were already there.
[...]
quoted
Use dprintk ?
Original dprintk or the DPRINTK used in my patch? If you mean DPRINTK,
Your.
then it wouldn't work, because DPRINTK includes dev->name. At this point
in the code, dev->name is not defined.
Perhaps I could modifying DPRINTK (*) to use dev->name if defined,
otherwise fall back on PFX.
I would put the smallest amount of things behind dprintk() so it can be
used anywhere (for consistency): no PFX, no dev->name.
Thanks for your work.
--
Ueimor
From: Jon Mason <hidden> Date: 2005-02-28 17:27:53
This patch increases the maximum MTU from ~7k to 8169 (the maximum MTU
that will fit into a single descriptor).
Applies cleanly to linux-2.6.11-rc4-mm1 and tested on amd64
Signed-off-by: Jon Mason <redacted>
@@ -117,8 +117,9 @@ static int multicast_filter_limit = 32;#define RX_DMA_BURST 6 /* Maximum PCI burst, '6' is 1024 */#define TX_DMA_BURST 6 /* Maximum PCI burst, '6' is 1024 */#define EarlyTxThld 0x3F /* 0x3F means NO early transmit */+#define LargeSendETT 0x35#define RxPacketMaxSize 0x3FE8 /* 16K - 1 - ETH_HLEN - VLAN - CRC... */-#define SafeMtu 0x1c20 /* ... actually life sucks beyond ~7k */+#define SafeMtu 0x1FE9 /* Largest MTU that can fit in a single desc */#define InterFrameGap 0x03 /* 3 means InterFrameGap = the shortest one */#define R8169_REGS_SIZE 256
@@ -1576,8 +1577,12 @@ rtl8169_hw_start(struct net_device *dev)RTL_W8(Cfg9346,Cfg9346_Unlock);RTL_W8(ChipCmd,CmdTxEnb|CmdRxEnb);-RTL_W8(EarlyTxThres,EarlyTxThld);+if(dev->mtu<7400)+RTL_W8(EarlyTxThres,EarlyTxThld);+else+RTL_W8(EarlyTxThres,LargeSendETT);+/* For gigabit rtl8169, MTU + header + CRC + VLAN */RTL_W16(RxMaxSize,tp->rx_buf_sz);
1 - Any objection against ternary operator, say:
RTL_W8(EarlyTxThres, (dev->mtu < 7400) ? EarlyTxThld : LargeSendETT);
2 - patch includes uneeded tabs on the last added (empty) line.
--
Ueimor
@@ -117,8 +117,9 @@ static int multicast_filter_limit = 32;#define RX_DMA_BURST 6 /* Maximum PCI burst, '6' is 1024 */#define TX_DMA_BURST 6 /* Maximum PCI burst, '6' is 1024 */#define EarlyTxThld 0x3F /* 0x3F means NO early transmit */+#define LargeSendETT 0x35#define RxPacketMaxSize 0x3FE8 /* 16K - 1 - ETH_HLEN - VLAN - CRC... */-#define SafeMtu 0x1c20 /* ... actually life sucks beyond ~7k */+#define SafeMtu 0x1FE9 /* Largest MTU that can fit in a single desc */#define InterFrameGap 0x03 /* 3 means InterFrameGap = the shortest one */#define R8169_REGS_SIZE 256
@@ -1576,7 +1577,7 @@ rtl8169_hw_start(struct net_device *dev)RTL_W8(Cfg9346,Cfg9346_Unlock);RTL_W8(ChipCmd,CmdTxEnb|CmdRxEnb);-RTL_W8(EarlyTxThres,EarlyTxThld);+RTL_W8(EarlyTxThres,(dev->mtu<7400)?EarlyTxThld:LargeSendETT);/* For gigabit rtl8169, MTU + header + CRC + VLAN */RTL_W16(RxMaxSize,tp->rx_buf_sz);
From: Jon Mason <hidden> Date: 2005-02-28 20:56:24
On Monday 28 February 2005 02:19 pm, Jeff Garzik wrote:
Random comment.
I think it's terribly cute that the max packet size is 8169. Why not
represent that in decimal, as opposed to hexidecimal?
Jeff
It actually random that it is 8169. Jumbo Frames MTU in r8169 is dependent on
the rx_buf_sz. In the 8169 case, the rx_buf_sz is actually 8191 - (ETH_HDR +
VLAN_HDR + CRC). Any rx_buf_sz > 8191 is split up into multiple descriptors,
which is proving to be a bit tricky.
--
Jon Mason
jdmason@us.ibm.com