Re: [PATCH] USB: pegasus uses netif_msg_*() filters
From: Jeff Garzik
Date: Wed Mar 30 2005 - 19:46:49 EST
Linux Kernel Mailing List wrote:
ChangeSet 1.2181.4.70, 2005/03/24 15:30:57-08:00, david-b@xxxxxxxxxxx
[PATCH] USB: pegasus uses netif_msg_*() filters
This updates the messaging for the pegasus driver:
- Use driver model diagnostics or printk using the interface name
for all diagnostic messages ... not dbg()/warn()/err().
- Almost everywhere, use the netif_msg_XXX() macros to check the
message control bitmask maintained by ethtool. The default mask
is initialized using a new "message_level" module parameter.
Also:
- Removes the needless PEGASUS_RUNNING flag, replacing it with the
standard netdevice mechanism.
- Cleaner access for unaligned values. Not all processors spend
silicon to support them like x86 does.
- Adds a few "sparse" fixes.
- Saves the return values for the requests that manipulate chip
registers ... doesn't yet check them, but at least anyone looking
at the code (e.g. to find out why the link check task is wedged...)
will see where those failure modes are ignored. Currently the
errors may be reported by printk, but the netif_msg_*() filters
make that an even worse alert mechanism.
Signed-off-by: David Brownell <dbrownell@xxxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxx>
pegasus.c | 297 +++++++++++++++++++++++++++++++++++++++-----------------------
pegasus.h | 4
2 files changed, 193 insertions(+), 108 deletions(-)
diff -Nru a/drivers/usb/net/pegasus.c b/drivers/usb/net/pegasus.c
--- a/drivers/usb/net/pegasus.c 2005-03-30 15:19:01 -08:00
+++ b/drivers/usb/net/pegasus.c 2005-03-30 15:19:01 -08:00
@@ -85,6 +85,11 @@
MODULE_PARM_DESC(loopback, "Enable MAC loopback mode (bit 0)");
MODULE_PARM_DESC(mii_mode, "Enable HomePNA mode (bit 0),default=MII mode = 0");
+/* use ethtool to change the level for any given device */
+static int msg_level = -1;
incorrect -- this should be an integer which indicates verbosity level;
netif_msg_init() then converts that into a bitmap.
ditto this comment for my previous email (forgot to mention this).
@@ -289,21 +309,23 @@
int i;
__u8 data[4] = { phy, 0, 0, indx };
__le16 regdi;
+ int ret;
- set_register(pegasus, PhyCtrl, 0);
- set_registers(pegasus, PhyAddr, sizeof (data), data);
- set_register(pegasus, PhyCtrl, (indx | PHY_READ));
+ ret = set_register(pegasus, PhyCtrl, 0);
+ ret = set_registers(pegasus, PhyAddr, sizeof (data), data);
+ ret = set_register(pegasus, PhyCtrl, (indx | PHY_READ));
for (i = 0; i < REG_TIMEOUT; i++) {
- get_registers(pegasus, PhyCtrl, 1, data);
+ ret = get_registers(pegasus, PhyCtrl, 1, data);
if (data[0] & PHY_DONE)
break;
}
if (i < REG_TIMEOUT) {
- get_registers(pegasus, PhyData, 2, ®di);
+ ret = get_registers(pegasus, PhyData, 2, ®di);
*regd = le16_to_cpu(regdi);
return 1;
}
these changes are completely pointless. please revert.
@@ -311,7 +333,7 @@
static int mdio_read(struct net_device *dev, int phy_id, int loc)
{
pegasus_t *pegasus = (pegasus_t *) netdev_priv(dev);
- __le16 res;
+ u16 res;
read_mii_word(pegasus, phy_id, loc, &res);
return (int)res;
pointless cast in last line (though I realize this wasn't actually
changed by your patch)
@@ -321,20 +343,23 @@
{
int i;
__u8 data[4] = { phy, 0, 0, indx };
+ int ret;
- *(data + 1) = cpu_to_le16p(®d);
- set_register(pegasus, PhyCtrl, 0);
- set_registers(pegasus, PhyAddr, sizeof(data), data);
- set_register(pegasus, PhyCtrl, (indx | PHY_WRITE));
+ data[1] = (u8) regd;
+ data[2] = (u8) (regd >> 8);
+ ret = set_register(pegasus, PhyCtrl, 0);
+ ret = set_registers(pegasus, PhyAddr, sizeof(data), data);
+ ret = set_register(pegasus, PhyCtrl, (indx | PHY_WRITE));
for (i = 0; i < REG_TIMEOUT; i++) {
- get_registers(pegasus, PhyCtrl, 1, data);
+ ret = get_registers(pegasus, PhyCtrl, 1, data);
if (data[0] & PHY_DONE)
break;
}
more pointless changes
@@ -350,23 +375,25 @@
int i;
__u8 tmp;
__le16 retdatai;
+ int ret;
- set_register(pegasus, EpromCtrl, 0);
- set_register(pegasus, EpromOffset, index);
- set_register(pegasus, EpromCtrl, EPROM_READ);
+ ret = set_register(pegasus, EpromCtrl, 0);
+ ret = set_register(pegasus, EpromOffset, index);
+ ret = set_register(pegasus, EpromCtrl, EPROM_READ);
for (i = 0; i < REG_TIMEOUT; i++) {
- get_registers(pegasus, EpromCtrl, 1, &tmp);
+ ret = get_registers(pegasus, EpromCtrl, 1, &tmp);
if (tmp & EPROM_DONE)
break;
}
if (i < REG_TIMEOUT) {
- get_registers(pegasus, EpromData, 2, &retdatai);
+ ret = get_registers(pegasus, EpromData, 2, &retdatai);
*retdata = le16_to_cpu(retdatai);
return 0;
}
and more
@@ -374,40 +401,44 @@
static inline void enable_eprom_write(pegasus_t * pegasus)
{
__u8 tmp;
+ int ret;
- get_registers(pegasus, EthCtrl2, 1, &tmp);
- set_register(pegasus, EthCtrl2, tmp | EPROM_WR_ENABLE);
+ ret = get_registers(pegasus, EthCtrl2, 1, &tmp);
+ ret = set_register(pegasus, EthCtrl2, tmp | EPROM_WR_ENABLE);
}
static inline void disable_eprom_write(pegasus_t * pegasus)
{
__u8 tmp;
+ int ret;
- get_registers(pegasus, EthCtrl2, 1, &tmp);
- set_register(pegasus, EpromCtrl, 0);
- set_register(pegasus, EthCtrl2, tmp & ~EPROM_WR_ENABLE);
+ ret = get_registers(pegasus, EthCtrl2, 1, &tmp);
+ ret = set_register(pegasus, EpromCtrl, 0);
+ ret = set_register(pegasus, EthCtrl2, tmp & ~EPROM_WR_ENABLE);
}
static int write_eprom_word(pegasus_t * pegasus, __u8 index, __u16 data)
{
int i;
__u8 tmp, d[4] = { 0x3f, 0, 0, EPROM_WRITE };
+ int ret;
- set_registers(pegasus, EpromOffset, 4, d);
+ ret = set_registers(pegasus, EpromOffset, 4, d);
enable_eprom_write(pegasus);
- set_register(pegasus, EpromOffset, index);
- set_registers(pegasus, EpromData, 2, &data);
- set_register(pegasus, EpromCtrl, EPROM_WRITE);
+ ret = set_register(pegasus, EpromOffset, index);
+ ret = set_registers(pegasus, EpromData, 2, &data);
+ ret = set_register(pegasus, EpromCtrl, EPROM_WRITE);
for (i = 0; i < REG_TIMEOUT; i++) {
- get_registers(pegasus, EpromCtrl, 1, &tmp);
+ ret = get_registers(pegasus, EpromCtrl, 1, &tmp);
if (tmp & EPROM_DONE)
break;
}
and more
@@ -426,9 +457,10 @@
static void set_ethernet_addr(pegasus_t * pegasus)
{
__u8 node_id[6];
+ int ret;
get_node_id(pegasus, node_id);
- set_registers(pegasus, EthID, sizeof (node_id), node_id);
+ ret = set_registers(pegasus, EthID, sizeof (node_id), node_id);
memcpy(pegasus->net->dev_addr, node_id, sizeof (node_id));
}
@@ -436,19 +468,20 @@
{
__u8 data = 0x8;
int i;
+ int ret;
- set_register(pegasus, EthCtrl1, data);
+ ret = set_register(pegasus, EthCtrl1, data);
for (i = 0; i < REG_TIMEOUT; i++) {
- get_registers(pegasus, EthCtrl1, 1, &data);
+ ret = get_registers(pegasus, EthCtrl1, 1, &data);
if (~data & 0x08) {
if (loopback & 1)
break;
if (mii_mode && (pegasus->features & HAS_HOME_PNA))
- set_register(pegasus, Gpio1, 0x34);
+ ret = set_register(pegasus, Gpio1, 0x34);
else
- set_register(pegasus, Gpio1, 0x26);
- set_register(pegasus, Gpio0, pegasus->features);
- set_register(pegasus, Gpio0, DEFAULT_GPIO_SET);
+ ret = set_register(pegasus, Gpio1, 0x26);
+ ret = set_register(pegasus, Gpio0, pegasus->features);
+ ret = set_register(pegasus, Gpio0, DEFAULT_GPIO_SET);
break;
}
}
@@ -457,8 +490,8 @@
if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
usb_dev_id[pegasus->dev_index].vendor == VENDOR_DLINK) {
- set_register(pegasus, Gpio0, 0x24);
- set_register(pegasus, Gpio0, 0x26);
+ ret = set_register(pegasus, Gpio0, 0x24);
+ ret = set_register(pegasus, Gpio0, 0x26);
}
if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_ELCON) {
__u16 auxmode;
@@ -474,6 +507,7 @@
__u16 linkpart;
__u8 data[4];
pegasus_t *pegasus = netdev_priv(dev);
+ int ret;
read_mii_word(pegasus, pegasus->phy, MII_LPA, &linkpart);
data[0] = 0xc9;
@@ -487,7 +521,7 @@
data[2] = (loopback & 1) ? 0x09 : 0x01;
memcpy(pegasus->eth_regs, data, sizeof (data));
- set_registers(pegasus, EthCtrl0, 3, data);
+ ret = set_registers(pegasus, EthCtrl0, 3, data);
if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
usb_dev_id[pegasus->dev_index].vendor == VENDOR_DLINK) {
and more and more and more
case -ENOENT:
case -ECONNRESET:
case -ESHUTDOWN:
- dbg("%s: rx unlink, %d", net->name, urb->status);
+ if (netif_msg_ifdown(pegasus))
+ pr_debug("%s: rx unlink, %d\n", net->name, urb->status);
inappropriate
- dbg("%s: tx unlink, %d", net->name, urb->status);
+ if (netif_msg_ifdown(pegasus))
+ pr_debug("%s: tx unlink, %d\n", net->name, urb->status);
return;
default:
ditto
@@ -731,7 +782,9 @@
/* some Pegasus-I products report LOTS of data
* toggle errors... avoid log spamming
*/
- pr_debug("%s: intr status %d\n", net->name, urb->status);
+ if (netif_msg_timer(pegasus))
+ pr_debug("%s: intr status %d\n", net->name,
+ urb->status);
}
if (urb->actual_length >= 6) {
avoid log spamming by net_ratelimit() or similar.
also, this seems unrelated to timer (inappropriate)
@@ -763,7 +816,7 @@
}
status = usb_submit_urb(urb, SLAB_ATOMIC);
- if (status)
+ if (status && netif_msg_timer(pegasus))
printk(KERN_ERR "%s: can't resubmit interrupt urb, %d\n",
net->name, status);
}
ditto?
@@ -771,7 +824,8 @@
static void pegasus_tx_timeout(struct net_device *net)
{
pegasus_t *pegasus = netdev_priv(net);
- printk(KERN_WARNING "%s: tx timeout\n", net->name);
+ if (netif_msg_timer(pegasus))
+ printk(KERN_WARNING "%s: tx timeout\n", net->name);
pegasus->tx_urb->transfer_flags |= URB_ASYNC_UNLINK;
usb_unlink_urb(pegasus->tx_urb);
pegasus->stats.tx_errors++;
inappropriate
@@ -822,8 +878,9 @@
static inline void disable_net_traffic(pegasus_t * pegasus)
{
int tmp = 0;
+ int ret;
- set_registers(pegasus, EthCtrl0, 2, &tmp);
+ ret = set_registers(pegasus, EthCtrl0, 2, &tmp);
}
static inline void get_interrupt_interval(pegasus_t * pegasus)
pointless
@@ -912,24 +971,32 @@
if (!pegasus->rx_skb)
return -ENOMEM;
- set_registers(pegasus, EthID, 6, net->dev_addr);
+ res = set_registers(pegasus, EthID, 6, net->dev_addr);
pointless
usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
usb_rcvbulkpipe(pegasus->usb, 1),
pegasus->rx_skb->data, PEGASUS_MTU + 8,
read_bulk_callback, pegasus);
- if ((res = usb_submit_urb(pegasus->rx_urb, GFP_KERNEL)))
- warn("%s: failed rx_urb %d", __FUNCTION__, res);
+ if ((res = usb_submit_urb(pegasus->rx_urb, GFP_KERNEL))) {
+ if (netif_msg_ifup(pegasus))
+ pr_debug("%s: failed rx_urb, %d", net->name, res);
+ goto exit;
+ }
+
usb_fill_int_urb(pegasus->intr_urb, pegasus->usb,
usb_rcvintpipe(pegasus->usb, 3),
pegasus->intr_buff, sizeof (pegasus->intr_buff),
intr_callback, pegasus, pegasus->intr_interval);
- if ((res = usb_submit_urb(pegasus->intr_urb, GFP_KERNEL)))
- warn("%s: failed intr_urb %d", __FUNCTION__, res);
- netif_start_queue(net);
- pegasus->flags |= PEGASUS_RUNNING;
+ if ((res = usb_submit_urb(pegasus->intr_urb, GFP_KERNEL))) {
+ if (netif_msg_ifup(pegasus))
+ pr_debug("%s: failed intr_urb, %d\n", net->name, res);
+ usb_kill_urb(pegasus->rx_urb);
+ goto exit;
+ }
if ((res = enable_net_traffic(net, pegasus->usb))) {
- err("can't enable_net_traffic() - %d", res);
+ if (netif_msg_ifup(pegasus))
+ pr_debug("%s: can't enable_net_traffic() - %d\n",
+ net->name, res);
res = -EIO;
usb_kill_urb(pegasus->rx_urb);
usb_kill_urb(pegasus->intr_urb);
inappropriate
@@ -937,6 +1004,9 @@
goto exit;
}
set_carrier(net);
+ netif_start_queue(net);
+ if (netif_msg_ifup(pegasus))
+ pr_debug("%s: open\n", net->name);
res = 0;
exit:
return res;
OK! (yay)
@@ -946,7 +1016,6 @@
{
pegasus_t *pegasus = netdev_priv(net);
- pegasus->flags &= ~PEGASUS_RUNNING;
netif_stop_queue(net);
if (!(pegasus->flags & PEGASUS_UNPLUG))
disable_net_traffic(pegasus);
where is the netif_msg_ifdown()?
@@ -1094,12 +1164,14 @@
if (net->flags & IFF_PROMISC) {
pegasus->eth_regs[EthCtrl2] |= RX_PROMISCUOUS;
- pr_info("%s: Promiscuous mode enabled.\n", net->name);
+ if (netif_msg_link(pegasus))
+ pr_info("%s: Promiscuous mode enabled.\n", net->name);
} else if ((net->mc_count > multicast_filter_limit) ||
(net->flags & IFF_ALLMULTI)) {
pegasus->eth_regs[EthCtrl0] |= RX_MULTICAST;
pegasus->eth_regs[EthCtrl2] &= ~RX_PROMISCUOUS;
- pr_info("%s: set allmulti\n", net->name);
+ if (netif_msg_link(pegasus))
+ pr_info("%s: set allmulti\n", net->name);
} else {
pegasus->eth_regs[EthCtrl0] &= ~RX_MULTICAST;
pegasus->eth_regs[EthCtrl2] &= ~RX_PROMISCUOUS;
inappropriate
@@ -1128,17 +1200,18 @@
static inline void setup_pegasus_II(pegasus_t * pegasus)
{
__u8 data = 0xa5;
+ int ret;
- set_register(pegasus, Reg1d, 0);
- set_register(pegasus, Reg7b, 1);
+ ret = set_register(pegasus, Reg1d, 0);
+ ret = set_register(pegasus, Reg7b, 1);
mdelay(100);
if ((pegasus->features & HAS_HOME_PNA) && mii_mode)
- set_register(pegasus, Reg7b, 0);
+ ret = set_register(pegasus, Reg7b, 0);
else
- set_register(pegasus, Reg7b, 2);
+ ret = set_register(pegasus, Reg7b, 2);
- set_register(pegasus, 0x83, data);
- get_registers(pegasus, 0x83, 1, &data);
+ ret = set_register(pegasus, 0x83, data);
+ ret = get_registers(pegasus, 0x83, 1, &data);
if (data == 0xa5) {
pegasus->chip = 0x8513;
more addition-of-dead-stores
@@ -1146,14 +1219,14 @@
pegasus->chip = 0;
}
- set_register(pegasus, 0x80, 0xc0);
- set_register(pegasus, 0x83, 0xff);
- set_register(pegasus, 0x84, 0x01);
+ ret = set_register(pegasus, 0x80, 0xc0);
+ ret = set_register(pegasus, 0x83, 0xff);
+ ret = set_register(pegasus, 0x84, 0x01);
if (pegasus->features & HAS_HOME_PNA && mii_mode)
- set_register(pegasus, Reg81, 6);
+ ret = set_register(pegasus, Reg81, 6);
else
- set_register(pegasus, Reg81, 2);
+ ret = set_register(pegasus, Reg81, 2);
}
ditto
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/