| 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: <20070513231029.GC28855@flint.arm.linux.org.uk>
Date: Mon, 14 May 2007 00:10:29 +0100
From: Russell King <rmk+lkml@....linux.org.uk>
To: Olof Johansson <olof@...om.net>
Cc: Christoph Hellwig <hch@...radead.org>,
linux-pcmcia@...ts.infradead.org, akpm@...l.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] pcmcia: export pcmcia_bus_type
Some random review comments on your driver, while I was passing. Spotted
a few things.
On Sun, May 13, 2007 at 04:40:07PM -0500, Olof Johansson wrote:
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/platform_device.h>
> +#include <linux/platform_device.h>
Silly duplicate includes.
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +
> +#include <pcmcia/ss.h>
> +#include <asm/of_platform.h>
> +
> +static const char driver_name[] = "electra-cf";
>...
> +static int electra_cf_get_status(struct pcmcia_socket *s, u_int *sp)
> +{
> + struct electra_cf_socket *cf;
> +
> + if (!sp)
> + return -EINVAL;
Never called with a NULL argument, so useless check.
> +
> + cf = container_of(s, struct electra_cf_socket, socket);
> +
> + /* NOTE CF is always 3VCARD */
> + if (electra_cf_present(cf)) {
> + struct electra_cf_socket *cf;
> +
> + *sp = SS_READY | SS_DETECT | SS_POWERON | SS_3VCARD;
> + cf = container_of(s, struct electra_cf_socket, socket);
> + s->pci_irq = cf->irq;
> + } else
> + *sp = 0;
> + return 0;
> +}
>...
> +static int electra_cf_ss_suspend(struct pcmcia_socket *s)
> +{
> + pr_debug("%s: %s\n", driver_name, __FUNCTION__);
> + return electra_cf_set_socket(s, &dead_socket);
> +}
That's already done for you. Remove this function entirely and set
the ".suspend" method to NULL.
> +static int electra_cf_set_io_map(struct pcmcia_socket *s,
> + struct pccard_io_map *io)
> +{
> + struct electra_cf_socket *cf;
> +
> + cf = container_of(s, struct electra_cf_socket, socket);
> + io->flags &= MAP_ACTIVE|MAP_ATTRIB|MAP_16BIT;
> + io->start = (unsigned long)cf->io_base;
> + io->stop = (unsigned long)cf->io_base + 0x800 - 1;
> + return 0;
> +}
PCMCIA code will ignore your writes to 'io'. Therefore, does this
do anything useful? If not, an empty function will do.
>...
> +static int __devinit electra_cf_probe(struct of_device *ofdev,
> + const struct of_device_id *match)
> +{
> + struct device *device = &ofdev->dev;
> + struct device_node *np = ofdev->node;
> + struct electra_cf_socket *cf;
> + struct resource mem, io;
> + int status;
> + const unsigned int *prop;
> + int err;
> +
> + err = of_address_to_resource(np, 0, &mem);
> + if (err)
> + return -EINVAL;
> +
> + err = of_address_to_resource(np, 1, &io);
> + if (err)
> + return -EINVAL;
> +
> + cf = kzalloc(sizeof *cf, GFP_KERNEL);
> + if (!cf)
> + return -ENOMEM;
> +
> + init_timer(&cf->timer);
> + cf->timer.function = electra_cf_timer;
> + cf->timer.data = (unsigned long) cf;
> +
> + cf->ofdev = ofdev;
> + cf->mem_phys = mem.start;
> + cf->mem_base = ioremap(mem.start, mem.end - mem.start);
> + cf->io_base = ioremap(io.start, io.end - io.start);
> + cf->gpio_base = ioremap(0xfc103000, 0x1000);
> + dev_set_drvdata(device, cf);
> +
> + if (!cf->mem_base || !cf->io_base || !cf->gpio_base) {
> + dev_err(device, "can't ioremap ranges\n");
> + status = -ENOMEM;
> + goto fail1;
> + }
> +
> + cf->iomem.start = (unsigned long)cf->io_base;
> + cf->iomem.end = (unsigned long)cf->io_base + (io.end - io.start);
> + cf->iomem.flags = IORESOURCE_MEM;
> +
> + cf->irq = irq_of_parse_and_map(np, 0);
> +
> + status = request_irq(cf->irq, electra_cf_irq, IRQF_SHARED,
> + driver_name, cf);
> + if (status < 0) {
> + dev_err(device, "request_irq failed\n");
> + goto fail1;
> + }
> +
> + cf->socket.pci_irq = cf->irq;
> +
> + prop = get_property(np, "card-detect-gpio", NULL);
> + cf->gpio_detect = *prop;
> + prop = get_property(np, "card-vsense-gpio", NULL);
> + cf->gpio_vsense = *prop;
> + prop = get_property(np, "card-3v-gpio", NULL);
> + cf->gpio_3v = *prop;
> + prop = get_property(np, "card-5v-gpio", NULL);
> + cf->gpio_5v = *prop;
> +
> + /* pcmcia layer only remaps "real" memory not iospace */
> + cf->socket.io_offset = (unsigned long)cf->io_base;
> +
> + /* reserve chip-select regions */
> + if (!request_mem_region(mem.start, mem.end + 1 - mem.start,
> + driver_name)) {
> + status = -ENXIO;
> + dev_err(device, "Can't claim memory region\n");
> + goto fail1;
> + }
> +
> + if (!request_mem_region(io.start, io.end + 1 - io.start,
> + driver_name)) {
> + status = -ENXIO;
> + dev_err(device, "Can't claim I/O region\n");
> + goto fail2;
> + }
> +
> + cf->socket.owner = THIS_MODULE;
> + cf->socket.dev.parent = &ofdev->dev;
> + cf->socket.ops = &electra_cf_ops;
> + cf->socket.resource_ops = &pccard_static_ops;
> + cf->socket.features = SS_CAP_PCCARD | SS_CAP_STATIC_MAP |
> + SS_CAP_MEM_ALIGN;
> + cf->socket.map_size = 0x800;
> + cf->socket.io[0].res = &cf->iomem;
Not a good idea - PCMCIA manages this resources itself and will free it
when the card is removed. Use ss_cap_static_map and socket.io_offset.
> +
> + status = pcmcia_register_socket(&cf->socket);
> + if (status < 0) {
> + dev_err(device, "pcmcia_register_socket failed\n");
> + goto fail3;
> + }
> +
> + dev_info(device, "at mem 0x%lx io 0x%lx irq %d\n",
> + mem.start, io.start, cf->irq);
> +
> + cf->active = 1;
> + electra_cf_timer((unsigned long)cf);
> + return 0;
> +
> +fail3:
> + release_mem_region(io.start, io.end + 1 - io.start);
> +fail2:
> + release_mem_region(mem.start, mem.end + 1 - mem.start);
> +fail1:
> + if (cf->io_base)
> + iounmap(cf->io_base);
> + if (cf->mem_base)
> + iounmap(cf->mem_base);
> + if (cf->gpio_base)
> + iounmap(cf->gpio_base);
> + device_init_wakeup(&ofdev->dev, 0);
> + kfree(cf);
> + return status;
> +
> +}
> +
> +static int __devexit electra_cf_remove(struct of_device *ofdev)
> +{
> + struct device *device = &ofdev->dev;
> + struct electra_cf_socket *cf;
> +
> + cf = dev_get_drvdata(device);
> +
> + cf->active = 0;
> + pcmcia_unregister_socket(&cf->socket);
> + del_timer_sync(&cf->timer);
> + free_irq(cf->irq, cf);
What if an IRQ occurs after the timer has been deleted? Doesn't the
interrupt handler re-add the timer back?
> +
> + iounmap(cf->io_base);
> + iounmap(cf->mem_base);
> + iounmap(cf->gpio_base);
> + release_mem_region(cf->mem_phys, cf->mem_size);
> + release_region(cf->io_phys, cf->io_size);
> +
> + kfree(cf);
> +
> + return 0;
> +}
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists