From: Jeff Garzik <jgarzik@pobox.com>
To: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
dbrownell@users.sourceforge.net, gregkh@suse.de
Subject: Re: [PATCH] USB: pegasus uses netif_msg_*() filters
Date: Wed, 30 Mar 2005 19:39:11 -0500 [thread overview]
Message-ID: <424B46AF.6080205@pobox.com> (raw)
In-Reply-To: <200503302318.j2UNImZh019630@hera.kernel.org>
Linux Kernel Mailing List wrote:
> ChangeSet 1.2181.4.70, 2005/03/24 15:30:57-08:00, david-b@pacbell.net
>
> [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@users.sourceforge.net>
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
>
>
>
> 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
next parent reply other threads:[~2005-03-31 0:43 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <200503302318.j2UNImZh019630@hera.kernel.org>
2005-03-31 0:39 ` Jeff Garzik [this message]
2005-03-31 1:11 ` David Brownell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=424B46AF.6080205@pobox.com \
--to=jgarzik@pobox.com \
--cc=dbrownell@users.sourceforge.net \
--cc=gregkh@suse.de \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®