| 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
| ||
|
Message-ID: <4D39F144.6090404@iki.fi>
Date: Fri, 21 Jan 2011 22:49:08 +0200
From: Sakari Ailus <sakari.ailus@....fi>
To: Joe Perches <joe@...ches.com>
CC: netdev@...r.kernel.org, Samuel Chessman <chessman@....org>
Subject: Re: [PATCH] tlan: Use pr_fmt, pr_<level> and netdev_<level>, remove
changelog
Joe Perches wrote:
> Neatening and standardization to the standard logging mechanisms.
> The changelog isn't useful anymore.
> Miscellaneous speen/speed typo correction.
Hi Joe,
Many thanks for the patch!
I definitely think it's good to replace pr_ prints with netdev_* macros.
I have a few other comments on your patch below.
> Signed-off-by: Joe Perches<joe@...ches.com>
> ---
>
> On top of Sakari Ailus' patches...
>
> drivers/net/tlan.c | 304 +++++++++++++---------------------------------------
> 1 files changed, 74 insertions(+), 230 deletions(-)
>
> diff --git a/drivers/net/tlan.c b/drivers/net/tlan.c
> index bbb0b12..ecfae1d 100644
> --- a/drivers/net/tlan.c
> +++ b/drivers/net/tlan.c
> @@ -25,153 +25,10 @@
> * Microchip Technology, 24C01A/02A/04A Data Sheet
> * available in PDF format from www.microchip.com
> *
> - * Change History
> - *
> - * Tigran Aivazian<tigran@....com>: TLan_PciProbe() now uses
> - * new PCI BIOS interface.
> - * Alan Cox <alan@...rguk.ukuu.org.uk>:
> - * Fixed the out of memory
> - * handling.
> - *
> - * Torben Mathiasen<torben.mathiasen@...paq.com> New Maintainer!
> - *
> - * v1.1 Dec 20, 1999 - Removed linux version checking
> - * Patch from Tigran Aivazian.
> - * - v1.1 includes Alan's SMP updates.
> - * - We still have problems on SMP though,
> - * but I'm looking into that.
> - *
> - * v1.2 Jan 02, 2000 - Hopefully fixed the SMP deadlock.
> - * - Removed dependency of HZ being 100.
> - * - We now allow higher priority timers to
> - * overwrite timers like TLAN_TIMER_ACTIVITY
> - * Patch from John Cagle<john.cagle@...paq.com>.
> - * - Fixed a few compiler warnings.
> - *
> - * v1.3 Feb 04, 2000 - Fixed the remaining HZ issues.
> - * - Removed call to pci_present().
> - * - Removed SA_INTERRUPT flag from irq handler.
> - * - Added __init and __initdata to reduce resisdent
> - * code size.
> - * - Driver now uses module_init/module_exit.
> - * - Rewrote init_module and tlan_probe to
> - * share a lot more code. We now use tlan_probe
> - * with builtin and module driver.
> - * - Driver ported to new net API.
> - * - tlan.txt has been reworked to reflect current
> - * driver (almost)
> - * - Other minor stuff
> - *
> - * v1.4 Feb 10, 2000 - Updated with more changes required after Dave's
> - * network cleanup in 2.3.43pre7 (Tigran& myself)
> - * - Minor stuff.
> - *
> - * v1.5 March 22, 2000 - Fixed another timer bug that would hang the
> - * driver if no cable/link were present.
> - * - Cosmetic changes.
> - * - TODO: Port completely to new PCI/DMA API
> - * Auto-Neg fallback.
> - *
> - * v1.6 April 04, 2000 - Fixed driver support for kernel-parameters.
> - * Haven't tested it though, as the kernel support
> - * is currently broken (2.3.99p4p3).
> - * - Updated tlan.txt accordingly.
> - * - Adjusted minimum/maximum frame length.
> - * - There is now a TLAN website up at
> - * http://hp.sourceforge.net/
> - *
> - * v1.7 April 07, 2000 - Started to implement custom ioctls. Driver now
> - * reports PHY information when used with Donald
> - * Beckers userspace MII diagnostics utility.
> - *
> - * v1.8 April 23, 2000 - Fixed support for forced speed/duplex settings.
> - * - Added link information to Auto-Neg and forced
> - * modes. When NIC operates with auto-neg the driver
> - * will report Link speed& duplex modes as well as
> - * link partner abilities. When forced link is used,
> - * the driver will report status of the established
> - * link.
> - * Please read tlan.txt for additional information.
> - * - Removed call to check_region(), and used
> - * return value of request_region() instead.
> - *
> - * v1.8a May 28, 2000 - Minor updates.
> - *
> - * v1.9 July 25, 2000 - Fixed a few remaining Full-Duplex issues.
> - * - Updated with timer fixes from Andrew Morton.
> - * - Fixed module race in TLan_Open.
> - * - Added routine to monitor PHY status.
> - * - Added activity led support for Proliant devices.
> - *
> - * v1.10 Aug 30, 2000 - Added support for EISA based tlan controllers
> - * like the Compaq NetFlex3/E.
> - * - Rewrote tlan_probe to better handle multiple
> - * bus probes. Probing and device setup is now
> - * done through TLan_Probe and TLan_init_one. Actual
> - * hardware probe is done with kernel API and
> - * TLan_EisaProbe.
> - * - Adjusted debug information for probing.
> - * - Fixed bug that would cause general debug
> - * information to be printed after driver removal.
> - * - Added transmit timeout handling.
> - * - Fixed OOM return values in tlan_probe.
> - * - Fixed possible mem leak in tlan_exit
> - * (now tlan_remove_one).
> - * - Fixed timer bug in TLan_phyMonitor.
> - * - This driver version is alpha quality, please
> - * send me any bug issues you may encounter.
> - *
> - * v1.11 Aug 31, 2000 - Do not try to register irq 0 if no irq line was
> - * set for EISA cards.
> - * - Added support for NetFlex3/E with nibble-rate
> - * 10Base-T PHY. This is untestet as I haven't got
> - * one of these cards.
> - * - Fixed timer being added twice.
> - * - Disabled PhyMonitoring by default as this is
> - * work in progress. Define MONITOR to enable it.
> - * - Now we don't display link info with PHYs that
> - * doesn't support it (level1).
> - * - Incresed tx_timeout beacuse of auto-neg.
> - * - Adjusted timers for forced speeds.
> - *
> - * v1.12 Oct 12, 2000 - Minor fixes (memleak, init, etc.)
> - *
> - * v1.13 Nov 28, 2000 - Stop flooding console with auto-neg issues
> - * when link can't be established.
> - * - Added the bbuf option as a kernel parameter.
> - * - Fixed ioaddr probe bug.
> - * - Fixed stupid deadlock with MII interrupts.
> - * - Added support for speed/duplex selection with
> - * multiple nics.
> - * - Added partly fix for TX Channel lockup with
> - * TLAN v1.0 silicon. This needs to be investigated
> - * further.
> - *
> - * v1.14 Dec 16, 2000 - Added support for servicing multiple frames per.
> - * interrupt. Thanks goes to
> - * Adam Keys<adam@...com>
> - * Denis Beaudoin<dbeaudoin@...com>
> - * for providing the patch.
> - * - Fixed auto-neg output when using multiple
> - * adapters.
> - * - Converted to use new taskq interface.
> - *
> - * v1.14a Jan 6, 2001 - Minor adjustments (spinlocks, etc.)
> - *
> - * Samuel Chessman<chessman@....org> New Maintainer!
> - *
> - * v1.15 Apr 4, 2002 - Correct operation when aui=1 to be
> - * 10T half duplex no loopback
> - * Thanks to Gunnar Eikman
> - *
> - * Sakari Ailus<sakari.ailus@....fi>:
> - *
> - * v1.15a Dec 15 2008 - Remove bbuf support, it doesn't work anyway.
> - * v1.16 Jan 6 2011 - Make checkpatch.pl happy.
> - * v1.17 Jan 6 2011 - Add suspend/resume support.
> - *
I agree with this. I just didn't think too much while writing my
patchset. :-)
> ******************************************************************************/
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> #include<linux/module.h>
> #include<linux/init.h>
> #include<linux/ioport.h>
> @@ -204,7 +61,7 @@ module_param_array(speed, int, NULL, 0);
> MODULE_PARM_DESC(aui, "ThunderLAN use AUI port(s) (0-1)");
> MODULE_PARM_DESC(duplex,
> "ThunderLAN duplex setting(s) (0-default, 1-half, 2-full)");
> -MODULE_PARM_DESC(speed, "ThunderLAN port speen setting(s) (0,10,100)");
> +MODULE_PARM_DESC(speed, "ThunderLAN port speed setting(s) (0,10,100)");
>
> MODULE_AUTHOR("Maintainer: Samuel Chessman<chessman@....org>");
> MODULE_DESCRIPTION("Driver for TI ThunderLAN based ethernet PCI adapters");
> @@ -542,7 +399,7 @@ static int __init tlan_probe(void)
> {
> int rc = -ENODEV;
>
> - printk(KERN_INFO "%s", tlan_banner);
> + pr_info("%s", tlan_banner);
>
> TLAN_DBG(TLAN_DEBUG_PROBE, "Starting PCI Probe....\n");
>
> @@ -551,16 +408,16 @@ static int __init tlan_probe(void)
> rc = pci_register_driver(&tlan_driver);
>
> if (rc != 0) {
> - printk(KERN_ERR "TLAN: Could not register pci driver.\n");
> + pr_err("Could not register pci driver\n");
> goto err_out_pci_free;
> }
>
> TLAN_DBG(TLAN_DEBUG_PROBE, "Starting EISA Probe....\n");
> tlan_eisa_probe();
>
> - printk(KERN_INFO "TLAN: %d device%s installed, PCI: %d EISA: %d\n",
> - tlan_devices_installed, tlan_devices_installed == 1 ? "" : "s",
> - tlan_have_pci, tlan_have_eisa);
> + pr_info("%d device%s installed, PCI: %d EISA: %d\n",
> + tlan_devices_installed, tlan_devices_installed == 1 ? "" : "s",
> + tlan_have_pci, tlan_have_eisa);
>
> if (tlan_devices_installed == 0) {
> rc = -ENODEV;
> @@ -619,7 +476,7 @@ static int __devinit tlan_probe1(struct pci_dev *pdev,
>
> rc = pci_request_regions(pdev, tlan_signature);
> if (rc) {
> - printk(KERN_ERR "TLAN: Could not reserve IO regions\n");
> + pr_err("Could not reserve IO regions\n");
I think that, now that we do have a struct device (pci_dev.dev)
reference, we should use dev_* macros.
I think I'll just resend my patchset with Ben's comments --- i.e. for
now I just remove my 2nd patch. The first one I'm keeping as-is since
it's important that this one gets in. Everything else will conflict with
that!
Ethtool interface support could perhaps be a topic for another set?
> goto err_out;
> }
> }
> @@ -627,7 +484,7 @@ static int __devinit tlan_probe1(struct pci_dev *pdev,
>
> dev = alloc_etherdev(sizeof(struct tlan_priv));
> if (dev == NULL) {
> - printk(KERN_ERR "TLAN: Could not allocate memory for device.\n");
> + pr_err("Could not allocate memory for device\n");
dev_err() also here.
> rc = -ENOMEM;
> goto err_out_regions;
> }
> @@ -646,8 +503,7 @@ static int __devinit tlan_probe1(struct pci_dev *pdev,
>
> rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> if (rc) {
> - printk(KERN_ERR
> - "TLAN: No suitable PCI mapping available.\n");
> + pr_err("No suitable PCI mapping available\n");
And here.
> goto err_out_free_dev;
> }
>
> @@ -661,7 +517,7 @@ static int __devinit tlan_probe1(struct pci_dev *pdev,
> }
> }
> if (!pci_io_base) {
> - printk(KERN_ERR "TLAN: No IO mappings available\n");
> + pr_err("No IO mappings available\n");
> rc = -EIO;
> goto err_out_free_dev;
> }
> @@ -717,13 +573,13 @@ static int __devinit tlan_probe1(struct pci_dev *pdev,
>
> rc = tlan_init(dev);
> if (rc) {
> - printk(KERN_ERR "TLAN: Could not set up device.\n");
> + pr_err("Could not set up device\n");
> goto err_out_free_dev;
> }
>
> rc = register_netdev(dev);
> if (rc) {
> - printk(KERN_ERR "TLAN: Could not register device.\n");
> + pr_err("Could not register device\n");
> goto err_out_uninit;
> }
>
> @@ -740,12 +596,11 @@ static int __devinit tlan_probe1(struct pci_dev *pdev,
> tlan_have_eisa++;
> }
>
> - printk(KERN_INFO "TLAN: %s irq=%2d, io=%04x, %s, Rev. %d\n",
> - dev->name,
> - (int) dev->irq,
> - (int) dev->base_addr,
> - priv->adapter->device_label,
> - priv->adapter_rev);
> + netdev_info(dev, "irq=%2d, io=%04x, %s, Rev. %d\n",
> + (int)dev->irq,
> + (int)dev->base_addr,
> + priv->adapter->device_label,
> + priv->adapter_rev);
> return 0;
>
> err_out_uninit:
> @@ -861,7 +716,7 @@ static void __init tlan_eisa_probe(void)
> }
>
> if (debug == 0x10)
> - printk(KERN_INFO "Found one\n");
> + pr_info("Found one\n");
>
>
> /* Get irq from board */
> @@ -890,12 +745,12 @@ static void __init tlan_eisa_probe(void)
>
> out:
> if (debug == 0x10)
> - printk(KERN_INFO "None found\n");
> + pr_info("None found\n");
> continue;
>
> out2:
> if (debug == 0x10)
> - printk(KERN_INFO "Card found but it is not enabled, skipping\n");
> + pr_info("Card found but it is not enabled, skipping\n");
> continue;
>
> }
> @@ -963,8 +818,7 @@ static int tlan_init(struct net_device *dev)
> priv->dma_size = dma_size;
>
> if (priv->dma_storage == NULL) {
> - printk(KERN_ERR
> - "TLAN: Could not allocate lists and buffers for %s.\n",
> + pr_err("Could not allocate lists and buffers for %s\n",
> dev->name);
> return -ENOMEM;
> }
> @@ -982,9 +836,8 @@ static int tlan_init(struct net_device *dev)
> (u8) priv->adapter->addr_ofs + i,
> (u8 *)&dev->dev_addr[i]);
> if (err) {
> - printk(KERN_ERR "TLAN: %s: Error reading MAC from eeprom: %d\n",
> - dev->name,
> - err);
> + pr_err("%s: Error reading MAC from eeprom: %d\n",
> + dev->name, err);
> }
> dev->addr_len = 6;
>
> @@ -1028,8 +881,8 @@ static int tlan_open(struct net_device *dev)
> dev->name, dev);
>
> if (err) {
> - pr_err("TLAN: Cannot open %s because IRQ %d is already in use.\n",
> - dev->name, dev->irq);
> + netdev_err(dev, "Cannot open because IRQ %d is already in use\n",
> + dev->irq);
> return err;
> }
>
> @@ -1512,8 +1365,8 @@ static u32 tlan_handle_tx_eof(struct net_device *dev, u16 host_int)
> }
>
> if (!ack)
> - printk(KERN_INFO
> - "TLAN: Received interrupt for uncompleted TX frame.\n");
> + netdev_info(dev,
> + "Received interrupt for uncompleted TX frame\n");
>
> if (eoc) {
> TLAN_DBG(TLAN_DEBUG_TX,
> @@ -1666,8 +1519,8 @@ drop_and_reuse:
> }
>
> if (!ack)
> - printk(KERN_INFO
> - "TLAN: Received interrupt for uncompleted RX frame.\n");
> + netdev_info(dev,
> + "Received interrupt for uncompleted RX frame\n");
>
>
> if (eoc) {
> @@ -1723,7 +1576,7 @@ drop_and_reuse:
>
> static u32 tlan_handle_dummy(struct net_device *dev, u16 host_int)
> {
> - pr_info("TLAN: Test interrupt on %s.\n", dev->name);
> + netdev_info(dev, "Test interrupt\n");
> return 1;
>
> }
> @@ -1816,7 +1669,7 @@ static u32 tlan_handle_status_check(struct net_device *dev, u16 host_int)
> if (host_int& TLAN_HI_IV_MASK) {
> netif_stop_queue(dev);
> error = inl(dev->base_addr + TLAN_CH_PARM);
> - pr_info("TLAN: %s: Adaptor Error = 0x%x\n", dev->name, error);
> + netdev_info(dev, "Adaptor Error = 0x%x\n", error);
> tlan_read_and_clear_stats(dev, TLAN_RECORD);
> outl(TLAN_HC_AD_RST, dev->base_addr + TLAN_HOST_CMD);
>
> @@ -2057,7 +1910,7 @@ static void tlan_reset_lists(struct net_device *dev)
> list->buffer[0].count = TLAN_MAX_FRAME_SIZE | TLAN_LAST_BUFFER;
> skb = netdev_alloc_skb_ip_align(dev, TLAN_MAX_FRAME_SIZE + 5);
> if (!skb) {
> - pr_err("TLAN: out of memory for received data.\n");
> + netdev_err(dev, "Out of memory for received data\n");
> break;
> }
>
> @@ -2141,13 +1994,13 @@ static void tlan_print_dio(u16 io_base)
> u32 data0, data1;
> int i;
>
> - pr_info("TLAN: Contents of internal registers for io base 0x%04hx.\n",
> - io_base);
> - pr_info("TLAN: Off. +0 +4\n");
> + pr_info("Contents of internal registers for io base 0x%04hx\n",
> + io_base);
> + pr_info("Off. +0 +4\n");
I think struct struct net_device could replace io_base as the argument.
Then we'd have struct net_device and could use netdev_info.
> for (i = 0; i< 0x4C; i += 8) {
> data0 = tlan_dio_read32(io_base, i);
> data1 = tlan_dio_read32(io_base, i + 0x4);
> - pr_info("TLAN: 0x%02x 0x%08x 0x%08x\n", i, data0, data1);
> + pr_info("0x%02x 0x%08x 0x%08x\n", i, data0, data1);
> }
>
> }
> @@ -2176,14 +2029,14 @@ static void tlan_print_list(struct tlan_list *list, char *type, int num)
Add struct net_device here as well.
> {
> int i;
>
> - pr_info("TLAN: %s List %d at %p\n", type, num, list);
> - pr_info("TLAN: Forward = 0x%08x\n", list->forward);
> - pr_info("TLAN: CSTAT = 0x%04hx\n", list->c_stat);
> - pr_info("TLAN: Frame Size = 0x%04hx\n", list->frame_size);
> + pr_info("%s List %d at %p\n", type, num, list);
> + pr_info(" Forward = 0x%08x\n", list->forward);
> + pr_info(" CSTAT = 0x%04hx\n", list->c_stat);
> + pr_info(" Frame Size = 0x%04hx\n", list->frame_size);
> /* for (i = 0; i< 10; i++) { */
> for (i = 0; i< 2; i++) {
> - pr_info("TLAN: Buffer[%d].count, addr = 0x%08x, 0x%08x\n",
> - i, list->buffer[i].count, list->buffer[i].address);
> + pr_info(" Buffer[%d].count, addr = 0x%08x, 0x%08x\n",
> + i, list->buffer[i].count, list->buffer[i].address);
> }
>
> }
> @@ -2398,7 +2251,7 @@ tlan_finish_reset(struct net_device *dev)
> if ((priv->adapter->flags& TLAN_ADAPTER_UNMANAGED_PHY) ||
> (priv->aui)) {
> status = MII_GS_LINK;
> - pr_info("TLAN: %s: Link forced.\n", dev->name);
> + netdev_info(dev, "Link forced\n");
> } else {
> tlan_mii_read_reg(dev, phy, MII_GEN_STS,&status);
> udelay(1000);
> @@ -2410,24 +2263,20 @@ tlan_finish_reset(struct net_device *dev)
> tlan_mii_read_reg(dev, phy, MII_AN_LPA,&partner);
> tlan_mii_read_reg(dev, phy, TLAN_TLPHY_PAR,&tlphy_par);
>
> - pr_info("TLAN: %s: Link active with ", dev->name);
> - if (!(tlphy_par& TLAN_PHY_AN_EN_STAT)) {
> - pr_info("forced 10%sMbps %s-Duplex\n",
> - tlphy_par& TLAN_PHY_SPEED_100
> - ? "" : "0",
> - tlphy_par& TLAN_PHY_DUPLEX_FULL
> - ? "Full" : "Half");
> - } else {
> - pr_info("Autonegotiation enabled, at 10%sMbps %s-Duplex\n",
> - tlphy_par& TLAN_PHY_SPEED_100
> - ? "" : "0",
> - tlphy_par& TLAN_PHY_DUPLEX_FULL
> - ? "Full" : "half");
> - pr_info("TLAN: Partner capability:");
> + netdev_info(dev,
> + "Link active with %s %uMbps %s-Duplex\n",
> + !(tlphy_par& TLAN_PHY_AN_EN_STAT)
> + ? "forced" : "Autonegotiation enabled,",
> + tlphy_par& TLAN_PHY_SPEED_100
> + ? 100 : 10,
> + tlphy_par& TLAN_PHY_DUPLEX_FULL
> + ? "Full" : "Half");
> + if (tlphy_par& TLAN_PHY_AN_EN_STAT) {
> + netdev_info(dev, "Partner capability:");
> for (i = 5; i< 10; i++)
> if (partner& (1<<i))
> - printk(" %s", media[i-5]);
> - printk("\n");
> + pr_cont(" %s", media[i-5]);
> + pr_cont("\n");
I think these prints could be removed by a separate patch before this
one. Would you like to do that, or shall I? :-)
No further comments on this one. Thanks.
Cheers,
--
Sakari Ailus
sakari.ailus@....fi
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists