Re: 2.6.11-rc2-mm1

[Evgeniy Polyakov]

On Tue, 2005-01-25 at 12:53 +0000, Christoph Hellwig wrote:
> Review of the superio subsystem sneaked in through bk-i2c.patch:

Hmm, "sneaked"... Good start, Christoph.

> diff -Nru a/drivers/superio/Kconfig b/drivers/superio/Kconfig
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/drivers/superio/Kconfig	2005-01-23 22:34:15 -08:00
> @@ -0,0 +1,56 @@
> +menu "SuperIO subsystem support"
> +
> +config SC_SUPERIO
> +	tristate "SuperIO subsystem support"
> +	depends on CONNECTOR
> +	help
> +	  SuperIO subsystem support.
> +	
> +	  This support is also available as a module.  If so, the module
> +          will be called superio.ko.
> 
> This doesn't mention what "SuperIO" is at all.  Also please skip the .ko
> postfix for the module name as the intree Kconfigs do.  The boilerplate has
> changed to:

Ok.

>   To compile this driver as a module, choose M here: the
>   module will be called <foo>.
> 
> diff -Nru a/drivers/superio/Makefile b/drivers/superio/Makefile
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/drivers/superio/Makefile	2005-01-23 22:34:15 -08:00
> @@ -0,0 +1,11 @@
> +#
> +# Makefile for the SuperIO subsystem.
> +#
> 
> Superflous.
> 
> +
> +obj-$(CONFIG_SC_SUPERIO)	+= superio.o
> +obj-$(CONFIG_SC_GPIO)		+= sc_gpio.o
> +obj-$(CONFIG_SC_ACB)		+= sc_acb.o
> +obj-$(CONFIG_SC_PC8736X)	+= pc8736x.o
> +obj-$(CONFIG_SC_SCX200)		+= scx200.o
> +
> +superio-objs		:= sc.o chain.o sc_conn.o
> 
> please use superio-y += so new conditional objects can be added more easily.

They must be added in the same file and line to allow easy control.
It is not directory like char/.

> diff -Nru a/drivers/superio/chain.c b/drivers/superio/chain.c
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/drivers/superio/chain.c	2005-01-23 22:34:15 -08:00
> @@ -0,0 +1,52 @@
> +/*
> + * 	chain.c
> 
> superfluos, the file name is obvious.  (Dito for all other files)
> 
> + * 
> + * 2004 Copyright (c) Evgeniy Polyakov <johnpol@xxxxxxxxxxx>
> + * All rights reserved.
> + * 
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + *
> + */
> +
> +#include <asm/atomic.h>
> +#include <asm/types.h>
> +
> +#include <linux/list.h>
> +#include <linux/slab.h>
> 
> always include <asm/*> after <linux/*> headers.  Please use <linux/types.h>
> istead of <asm/types.h> always.
> 
> (comment applies to later files aswell)

Ok.

> +#include "chain.h"
> +
> +struct dev_chain *chain_alloc(void *ptr)
> +{
> +	struct dev_chain *ch;
> +
> +	ch = kmalloc(sizeof(struct dev_chain), GFP_ATOMIC);
> +	if (!ch) {
> +		printk(KERN_ERR "Failed to allocate new chain for %p.\n", ptr);
> +		return NULL;
> +	}
> +
> +	memset(ch, 0, sizeof(struct dev_chain));
> +
> +	ch->ptr = ptr;
> +
> +	return ch;
> +}
> +
> +void chain_free(struct dev_chain *ch)
> +{
> +	memset(ch, 0, sizeof(struct dev_chain));
> +	kfree(ch);
> 
> The memset completely defeats slab redzoning to catch bugs, don't
> do that.

What? Does following code also kills redzoning?

int *a;
a = kmalloc();
if (a)
{
	memset(a, 0, sizeof(*a));
	kfree(a);
}

Consider size of the dev_chain structure...

> Also what's the reason you can't simply put the list_head into struct
> logical_dev?

Because it is not just list_head, but special structure used for special
pointer manipulations,
which you are obviously saw in sc.c 

> +static void pc8736x_fini(void)
> +{
> +	sc_del_sc_dev(&pc8736x_dev);
> +
> +	while (atomic_read(&pc8736x_dev.refcnt)) {
> +		printk(KERN_INFO "Waiting for %s to became free: refcnt=%d.\n",
> +				pc8736x_dev.name, atomic_read(&pc8736x_dev.refcnt));
> +		
> +		set_current_state(TASK_INTERRUPTIBLE);
> +		schedule_timeout(HZ);
> +			
> +		if (current->flags & PF_FREEZE)
> +			refrigerator(PF_FREEZE);
> +
> +		if (signal_pending(current))
> +			flush_signals(current);
> +	}
> +}
> 
> And who gurantess this won't deadlock?  Please use a dynamically allocated
> driver model device and it's refcounting, thanks.

Sigh.

Christoph, please read the code before doing such comments.
I very respect your review and opinion, but only until you respect
others.

> +int sc_add_sc_dev(struct sc_dev *__sdev)
> 
> btw, what's the reason you use those ugly __ names for local variables all over?
> 
> +	while (atomic_read(&__sdev->refcnt)) {
> +		printk(KERN_INFO "Waiting SuperIO chip %s to become free: refcnt=%d.\n",
> +		       __sdev->name, atomic_read(&__sdev->refcnt));
> +		set_current_state(TASK_UNINTERRUPTIBLE);
> +		schedule_timeout(HZ);
> +			
> +		if (current->flags & PF_FREEZE)
> +			refrigerator(PF_FREEZE);
> +
> +		if (signal_pending(current))
> +			flush_signals(current);
> +	}
> +}
> 
> Again as above.
> 
> +static void sc_deactivate_logical(struct sc_dev *dev, struct logical_dev *ldev)
> +{
> +	printk(KERN_INFO "Deactivating logical device %s in SuperIO chip %s... ",
> +	       ldev->name, dev->name);
> +	
> +	if (ldev->irq)
> +	{
> +		free_irq(ldev->irq, ldev);
> +		ldev->irq = 0;
> +	}
> 
> CodingStyle: if (ldev->irq) {  (also in various other places)

Yep.

> +static int __devinit sc_init(void)
> +{
> +	printk(KERN_INFO "SuperIO driver is starting...\n");
> +
> +	return sc_register_callback();
> +}
> +
> +static void __devexit sc_fini(void)
> +{
> +	sc_unregister_callback();
> +	printk(KERN_INFO "SuperIO driver finished.\n");
> +}
> 
> quite noise.  Please only print messages when you find an actual
> device and not on unloading at all.
> 
> +	INIT_LIST_HEAD(&ldev_acb.ldev_entry);
> +	spin_lock_init(&ldev_acb.lock);
> 
> these two can be initialized at compile time.

Ok.

> +#include "../superio/sc.h"
> +#include "../superio/sc_gpio.h"
> 
> just include them normalluy, ok?

Sure.

> +static int scx200_pci_probe(struct pci_dev *pdev,
> +			    const struct pci_device_id *ent)
> +{
> +	private_base = pci_resource_start(pdev, 0);
> +	printk(KERN_INFO "%s: GPIO base 0x%lx.\n", pci_name(pdev), private_base);
> +
> +	if (!request_region
> +	    (private_base, SCx200_GPIO_SIZE, "NatSemi SCx200 GPIO")) {
> +		printk(KERN_ERR "%s: failed to request %d bytes I/O region for GPIOs.\n",
> +		       pci_name(pdev), SCx200_GPIO_SIZE);
> +		return -EBUSY;
> +	}
> +
> +	pci_set_drvdata(pdev, &private_base);
> +	pci_enable_device(pdev);
> 
> pci_enable_device needs to be done first, and it returns and error that
> should be handled.

Yep, it can be a problem, thank you.

> +	pci_unregister_driver(&scx200_pci_driver);
> +	if (private_base)
> +		release_region(private_base, SCx200_GPIO_SIZE);
> 
> this must happen in the ->remove callback.
> 
> diff -Nru a/drivers/superio/scx200.h b/drivers/superio/scx200.h
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/drivers/superio/scx200.h	2005-01-23 22:34:15 -08:00
> @@ -0,0 +1,28 @@
> +/*
> + * 	scx200.h
> + * 
> + * 2004 Copyright (c) Evgeniy Polyakov <johnpol@xxxxxxxxxxx>
> + * All rights reserved.
> + * 
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + *
> + */
> +
> +#ifndef __SCX200_H
> +#define __SCX200_H
> +
> +#define SCx200_GPIO_SIZE 	0x2c
> +
> +#endif /* __SCX200_H */
> 
> Yeah, right - a 30 line header for a single define that's used in a
> single source file..

Christoph, do you know what SuperIO is?
I doubt...

It is a small chip, which can include various number of devices.
SuperIO currently supports only GPIO and ACB, so this header only
includes
one define. I do not have hardware(sc1100 based for example) that
"exports"
other devices and which can be accessed from the outside of the board, 
so I did not add other defines.

But specially for you I can remove this file, will it satisfy you?

> Also your locking is broken.  sdev_lock sometimes nests outside
> sdev->lock and sometimes inside.  Similarly dev->chain_lock nests
> inside dev->lock sometimes and sometimes outside.  You really need
> a locking hiearchy document and the lockign should probably be
> simplified a lot.

It is almost the same like after hand waving say that there is a wind.

Each lock protect it's own data, sometimes it happens when other data is
locked, 
sometimes not. Yes, probably interrupt handling can race, it requires
more review,
I will take a look.

Below is part of the announce in lm_sensors@ which probably will throw
light on your
claims.
************
Main goal was to be able to link any registered SuperIO chip with any
number of registered logical devices. Next step is to add ability to
communicate with SuperIO subsystem from userspace.
External kernel modules may access to logical devices using
sc_{get,put}_ldev() and in similar way to SuperIO chip drivers -
sc_{get,put}_sdev().

Driver writers shoud use sc_{add,del}_logical_dev() and
sc_{add,del}_sc_dev() to accordingly add/remove logical devices and
SuperIO chip drivers.

Any SuperIO chip driver must provite at least 2 functions:
->probe() and ->activate_one().
The former is called with different HW addresses and should return 0 if
device was found there or negative error value.
The latter is called each time new logical device being added with
pointer to it's logical device structure. It should return 0 if logical
device was found to be active or negative error value.

Any logical device must provide 4 functions:
->activate() - it is used to activate logical device.
->read()/write() - what do you think, it is used to read and write
from/to logical device registers.
->control() - it is used to control access to logical device. Actually
it is private combination of ->read()/write() functions but is providede
for convenience.
********************

Resume:
Cristoph, you rudely try to show that this code is badly broken.
It is not.
It was tested as opposed to your claims, and works as expected.

To be more productive please next time be polite and respect others, 
or you will be just ignored no matter how strong your knowledges are.

Talk is cheap, show me the code, that is broken, and I will fix it.

Thank you.

-- 
        Evgeniy Polyakov

Crash is better than data corruption -- Arthur Grabowski

Attachment: signature.asc
Description: This is a digitally signed message part