lkml.org 
[lkml]   [2005]   [Jan]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: 2.6.11-rc2-mm1
From
Date
On Tue, 2005-01-25 at 14:23 +0000, Christoph Hellwig wrote:
> > > +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/.
>
> Huh?
>
> What I mean is you should write
>
> superio-y += sc.o chain.o sc_conn.o
>
> this allows adding things like
>
> superio-$(CONFIG_FOO) += sc_foo.o
>
> and is generally the canonical form since 2.6

Yes, I understand you, but this is the case when it is not supposed to
have external dependent features, they are already inside :)
But who knows.

I think it is better to change.

> > > +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...
>
> Sorry, didn't mean redzoning but poisoning in general, little
> brainfart on my side. The slab code can set freed objects to
> known patters so use after frees can be debugged easily, and
> by zeroing a structure before freeing we lose that.

Zeroing can be found easily - the whole structure is NULL pointer,
and will always panic if accessed(from running superio code),
but redzoning is only happen on borders,
and can catch writes over the boards, which is rarely in this case.

> > > 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
>
> No, I didn't see it. I see that the void pointer ptr in struct dev_chain
> always points to a struct sc_dev *, and I see we never change that
> pointer at run time. I might have missed something obvious, so maybe
> you could point me to it (or even better add a comment describing it)

Each superio chip is registered with superio core without any devices.
Each logical device is registered with superio core without any superio
chip.
Then core checks if some chip is found in some superio device, and
links
it to appropriate device.
So if board has several superio chips(like soekris board) and several
logical devices in it, then we only have 2 superio small drivers, and
several
logical device drivers, but not
number_of_superio_chips*number_of_logical_devices drivers.
Without dev_chain structure each logical device should have a separate
list of
superio chips that owns it.
Above chain structure is a glue between superio core, logical devices
and superio chips.
It is especially usefull in case of logical device cloning, when chip
does not follow
the standart and place it somewhere else, but to allow access to it
superio core
"clones" logical device and link them instead of the standart one.

> >
> > > +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.
>
> The code above pretty much means you can keep rmmod stalled forever.

Yes, and it is better than removing module whose structures are in use.
SuperIO core is asynchronous in it's nature, one can use logical device
through superio core and remove it's module on other CPU, above loop
will
wait untill all reference counters are dropped.

Access to devices can be done not through ioctl or such mechanism which
"locks" the module owner.

It is better than module_get() since with the latter variant we may end
up
not removing device at all.

> Also it seems to be the only code intree doing refrigerator() on anything
> but kernel thread. While I can't comment on swsusp internals it surely
> looks buggy to me.
>
> > > +#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?
>
> I've just told you it looks extremly silly, you need to decide on your
> own whether it's worthwile.
>
> > > 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.
>
> The thing I mention is called lock order reversal, which means a deadlock
> in most cases. I don't have the time to actual walk through all codepathes
> to tell you whether it can really happen and where, but it's a really
> big warning sign.

No, it is not called lock order reversal.

There are no places like
lock a
lock b
unlock a
unlock b

and if they are, then I'm completely wrong.

What you see is only following:

place 1:
lock a
lock b
unlock b
lock c
unlock c
unlock a

place 2:
lock b
lock a
unlock a
lock c
unlock c
unlock b


It is done since manipulation with the chain pointer can be done from
logical device structure and from superio chip structure, and this
operations are absolutely the same in nature.

> > 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.
>
> I've seen tons of code that "works as expected" but still is buggy.
> That's why code needs to be both tested (with a workload as
> expected and other stress testing that shows it handles loads _not_
> expected) and reviewed for errors that don't happen with a normal
> load or design problems.

Yes, you are right, of course.
That is why we write code, test and discuss(but not knock against each
other) it.

I will send patch to address your comments soon, thank you.

--
Evgeniy Polyakov

Crash is better than data corruption -- Arthur Grabowski
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2005-03-22 14:09    [W:0.117 / U:0.764 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site