| 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: <20090601220854.a9ddd5ce.akpm@linux-foundation.org>
Date: Mon, 1 Jun 2009 22:08:54 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: Florian Fainelli <florian@...nwrt.org>
Cc: linux-kernel@...r.kernel.org, Ralf Baechle <ralf@...ux-mips.org>,
Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH 2/9] add support for the TI VLYNQ bus
On Mon, 1 Jun 2009 13:58:27 +0200 Florian Fainelli <florian@...nwrt.org> wrote:
> This patch adds support for the TI VLYNQ high-speed,
> serial and packetized bus. This bus allows external
> devices to be connected to the System-on-Chip and
> appear in the main system memory just like any memory
> mapped peripheral. It is widely used in TI's networking
> and mutlimedia SoC, including the AR7 SoC.
>
>
> ...
>
> +struct vlynq_regs {
> + u32 revision;
> + u32 control;
> + u32 status;
> + u32 int_prio;
> + u32 int_status;
> + u32 int_pending;
> + u32 int_ptr;
> + u32 tx_offset;
> + struct vlynq_mapping rx_mapping[4];
> + u32 chip;
> + u32 autonego;
> + u32 unused[6];
> + u32 int_device[8];
> +};
> +
> +#define vlynq_reg_read(reg) readl(&(reg))
> +#define vlynq_reg_write(reg, val) writel(val, &(reg))
grumble. These just make the code harder to follow. it'd be better to
open-code readl() and writel() at the callsites.
> +static int __vlynq_enable_device(struct vlynq_device *dev);
> +
> +#ifdef VLYNQ_DEBUG
> +static void vlynq_dump_regs(struct vlynq_device *dev)
> +{
> + int i;
> +
> + printk(KERN_DEBUG "VLYNQ local=%p remote=%p\n",
> + dev->local, dev->remote);
> + for (i = 0; i < 32; i++) {
> + printk(KERN_DEBUG "VLYNQ: local %d: %08x\n",
> + i + 1, ((u32 *)dev->local)[i]);
> + printk(KERN_DEBUG "VLYNQ: remote %d: %08x\n",
> + i + 1, ((u32 *)dev->remote)[i]);
> + }
> +}
> +
> +static void vlynq_dump_mem(u32 *base, int count)
> +{
> + int i;
> +
> + for (i = 0; i < (count + 3) / 4; i++) {
> + if (i % 4 == 0)
> + printk(KERN_DEBUG "\nMEM[0x%04x]:", i * 4);
> + printk(KERN_DEBUG " 0x%08x", *(base + i));
> + }
> + printk(KERN_DEBUG "\n");
> +}
lib/hexdump.c?
> +#endif
> +
> +int vlynq_linked(struct vlynq_device *dev)
afacit this didn't need to be a kernel-wide symbol?
Please review the patchset for any unnecessarily-global symbols.
> +{
> + int i;
> +
> + for (i = 0; i < 100; i++)
> + if (vlynq_reg_read(dev->local->status) & VLYNQ_STATUS_LINK)
> + return 1;
> + else
> + cpu_relax();
> +
> + return 0;
> +}
> +
>
> ...
>
> +static void vlynq_remote_ack(unsigned int irq)
> +{
> + struct vlynq_device *dev = get_irq_chip_data(irq);
> +
> + u32 status = vlynq_reg_read(dev->remote->status);
> +
> + if (printk_ratelimit())
> + printk(KERN_DEBUG "%s: remote status: 0x%08x\n",
> + dev_name(&dev->dev), status);
> + vlynq_reg_write(dev->remote->status, status);
> +}
This code seems to do rather a lot of printks for production code?
>
> ...
>
> +static int vlynq_device_match(struct device *dev,
> + struct device_driver *drv)
> +{
> + struct vlynq_device *vdev = to_vlynq_device(dev);
> + struct vlynq_driver *vdrv = to_vlynq_driver(drv);
> + struct vlynq_device_id *ids = vdrv->id_table;
> +
> + while (ids->id) {
> + if (ids->id == vdev->dev_id) {
> + vdev->divisor = ids->divisor;
> + vlynq_set_drvdata(vdev, ids);
> + printk(KERN_INFO "Driver found for VLYNQ " \
> + "device: %08x\n", vdev->dev_id);
> + return 1;
> + }
> + printk(KERN_DEBUG "Not using the %08x VLYNQ device's driver" \
> + " for VLYNQ device: %08x\n", ids->id, vdev->dev_id);
The backslashes here are unneeded and unconventional.
> + ids++;
> + }
> + return 0;
> +}
> +
> +static int vlynq_device_probe(struct device *dev)
> +{
> + struct vlynq_device *vdev = to_vlynq_device(dev);
> + struct vlynq_driver *drv = to_vlynq_driver(dev->driver);
> + struct vlynq_device_id *id = vlynq_get_drvdata(vdev);
> + int result = -ENODEV;
> +
> + get_device(dev);
> + if (drv && drv->probe)
> + result = drv->probe(vdev, id);
Can `drv' really be NULL here?
> + if (result)
> + put_device(dev);
> + return result;
> +}
> +
> +static int vlynq_device_remove(struct device *dev)
> +{
> + struct vlynq_driver *drv = to_vlynq_driver(dev->driver);
> + if (drv && drv->remove)
> + drv->remove(to_vlynq_device(dev));
ditto.
> + put_device(dev);
> + return 0;
> +}
> +
> +int __vlynq_register_driver(struct vlynq_driver *driver, struct module *owner)
> +{
> + driver->driver.name = driver->name;
> + driver->driver.bus = &vlynq_bus_type;
> + return driver_register(&driver->driver);
> +}
> +EXPORT_SYMBOL(__vlynq_register_driver);
> +
> +void vlynq_unregister_driver(struct vlynq_driver *driver)
> +{
> + driver_unregister(&driver->driver);
> +}
> +EXPORT_SYMBOL(vlynq_unregister_driver);
> +
> +static int __vlynq_try_remote(struct vlynq_device *dev)
> +{
> + int i;
> +
> + vlynq_reset(dev);
> + for (i = dev->dev_id ? vlynq_rdiv2 : vlynq_rdiv8; dev->dev_id ?
> + i <= vlynq_rdiv8 : i >= vlynq_rdiv2;
> + dev->dev_id ? i++ : i--) {
Wow.
> + if (!vlynq_linked(dev))
> + break;
> +
> + vlynq_reg_write(dev->remote->control,
> + (vlynq_reg_read(dev->remote->control) &
> + ~VLYNQ_CTRL_CLOCK_MASK) |
> + VLYNQ_CTRL_CLOCK_INT |
> + VLYNQ_CTRL_CLOCK_DIV(i - vlynq_rdiv1));
> + vlynq_reg_write(dev->local->control,
> + ((vlynq_reg_read(dev->local->control)
> + & ~(VLYNQ_CTRL_CLOCK_INT |
> + VLYNQ_CTRL_CLOCK_MASK)) |
> + VLYNQ_CTRL_CLOCK_DIV(i - vlynq_rdiv1)));
> +
> + if (vlynq_linked(dev)) {
> + printk(KERN_DEBUG
> + "%s: using remote clock divisor %d\n",
> + dev_name(&dev->dev), i - vlynq_rdiv1 + 1);
> + dev->divisor = i;
> + return 0;
> + } else {
> + vlynq_reset(dev);
> + }
> + }
> +
> + return -ENODEV;
> +}
This code could do with a few comments.
See, this reader of your code doesn't know what vlynq_linked() does, so
I don't have a hope of working out whey this function returns -ENODEV
if it couldn't find any "linked" devices. What does this _mean_ in
terms of the underlying hardware and its configuration?
Dunno. But it would be nice to be able to work that out from reading
the code, no?
> +static int __vlynq_try_local(struct vlynq_device *dev)
> +{
> + int i;
> +
> + vlynq_reset(dev);
> +
> + for (i = dev->dev_id ? vlynq_ldiv2 : vlynq_ldiv8; dev->dev_id ?
> + i <= vlynq_ldiv8 : i >= vlynq_ldiv2;
> + dev->dev_id ? i++ : i--) {
> +
> + vlynq_reg_write(dev->local->control,
> + (vlynq_reg_read(dev->local->control) &
> + ~VLYNQ_CTRL_CLOCK_MASK) |
> + VLYNQ_CTRL_CLOCK_INT |
> + VLYNQ_CTRL_CLOCK_DIV(i - vlynq_ldiv1));
> +
> + if (vlynq_linked(dev)) {
> + printk(KERN_DEBUG
> + "%s: using local clock divisor %d\n",
> + dev_name(&dev->dev), i - vlynq_ldiv1 + 1);
> + dev->divisor = i;
> + return 0;
> + } else {
> + vlynq_reset(dev);
> + }
> + }
> +
> + return -ENODEV;
> +}
> +
> +static int __vlynq_try_external(struct vlynq_device *dev)
> +{
> + vlynq_reset(dev);
> + if (!vlynq_linked(dev))
> + return -ENODEV;
> +
> + vlynq_reg_write(dev->remote->control,
> + (vlynq_reg_read(dev->remote->control) &
> + ~VLYNQ_CTRL_CLOCK_INT));
> +
> + vlynq_reg_write(dev->local->control,
> + (vlynq_reg_read(dev->local->control) &
> + ~VLYNQ_CTRL_CLOCK_INT));
> +
> + if (vlynq_linked(dev)) {
> + printk(KERN_DEBUG "%s: using external clock\n",
> + dev_name(&dev->dev));
> + dev->divisor = vlynq_div_external;
> + return 0;
> + }
> +
> + return -ENODEV;
> +}
"remote", "local", "external". What do these terms mean? Perhaps
there's a TI datasheet somewhere?
> +static int __vlynq_enable_device(struct vlynq_device *dev)
> +{
> + int result;
> + struct plat_vlynq_ops *ops = dev->dev.platform_data;
> +
> + result = ops->on(dev);
> + if (result)
> + return result;
> +
> + switch (dev->divisor) {
> + case vlynq_div_external:
> + case vlynq_div_auto:
> + /* When the device is brought from reset it should have clock
> + generation negotiated by hardware.
> + Check which device is generating clocks and perform setup
> + accordingly */
Preferred comment layout format is
/*
* When the device is brought from reset it should have clock
* generation negotiated by hardware. Check which device is
* generating clocks and perform setup accordingly
*/
> + if (vlynq_linked(dev) && vlynq_reg_read(dev->remote->control) &
> + VLYNQ_CTRL_CLOCK_INT) {
> + if (!__vlynq_try_remote(dev) ||
> + !__vlynq_try_local(dev) ||
> + !__vlynq_try_external(dev))
> + return 0;
> + } else {
> + if (!__vlynq_try_external(dev) ||
> + !__vlynq_try_local(dev) ||
> + !__vlynq_try_remote(dev))
> + return 0;
> + }
> + break;
> + case vlynq_ldiv1: case vlynq_ldiv2: case vlynq_ldiv3: case vlynq_ldiv4:
> + case vlynq_ldiv5: case vlynq_ldiv6: case vlynq_ldiv7: case vlynq_ldiv8:
One `case' per line, please.
> + vlynq_reg_write(dev->local->control,
> + VLYNQ_CTRL_CLOCK_INT |
> + VLYNQ_CTRL_CLOCK_DIV(dev->divisor -
> + vlynq_ldiv1));
> + vlynq_reg_write(dev->remote->control, 0);
> + if (vlynq_linked(dev)) {
> + printk(KERN_DEBUG
> + "%s: using local clock divisor %d\n",
> + dev_name(&dev->dev),
> + dev->divisor - vlynq_ldiv1 + 1);
> + return 0;
> + }
> + break;
> + case vlynq_rdiv1: case vlynq_rdiv2: case vlynq_rdiv3: case vlynq_rdiv4:
> + case vlynq_rdiv5: case vlynq_rdiv6: case vlynq_rdiv7: case vlynq_rdiv8:
> + vlynq_reg_write(dev->local->control, 0);
> + vlynq_reg_write(dev->remote->control,
> + VLYNQ_CTRL_CLOCK_INT |
> + VLYNQ_CTRL_CLOCK_DIV(dev->divisor -
> + vlynq_rdiv1));
> + if (vlynq_linked(dev)) {
> + printk(KERN_DEBUG
> + "%s: using remote clock divisor %d\n",
> + dev_name(&dev->dev),
> + dev->divisor - vlynq_rdiv1 + 1);
> + return 0;
> + }
> + break;
> + }
> +
> + ops->off(dev);
> + return -ENODEV;
> +}
> +
> +int vlynq_enable_device(struct vlynq_device *dev)
> +{
> + struct plat_vlynq_ops *ops = dev->dev.platform_data;
> + int result = -ENODEV;
> +
> + result = __vlynq_enable_device(dev);
> + if (result)
> + return result;
> +
> + result = vlynq_setup_irq(dev);
> + if (result)
> + ops->off(dev);
It's strange that this function directly calls __vlynq_enable_device(),
and undoes that via ops->off() if vlynq_setup_irq() failed.
I'd have expected to see a call to ops->off() used as a cancallation
for ops->on()?
> + dev->enabled = !result;
> + return result;
> +}
> +EXPORT_SYMBOL(vlynq_enable_device);
> +
> +
>
> ...
>
> +struct vlynq_device {
> + u32 id, dev_id;
> + int local_irq;
> + int remote_irq;
> + enum vlynq_divisor divisor;
> + u32 regs_start, regs_end;
> + u32 mem_start, mem_end;
This doesn't look 64-bit-bus friendly.
> + u32 irq_start, irq_end;
> + int irq;
> + int enabled;
> + struct vlynq_regs *local;
> + struct vlynq_regs *remote;
> + struct device dev;
> +};
>
> ...
>
--
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