Re: [PATCH] SuperHyway bus support

[Paul Mundt]

On Thu, Jan 06, 2005 at 11:22:22PM -0800, Greg KH wrote:
> So it looks like you are just adding a fake "bus" driver and then
> manually adding the bus devices to the root device.  Why not just
> complete the conversion and register the real devices properly?
> 
Can you elaborate on what you mean by registering the "real devices properly"?

> We are depreciating device lists within the kernel.  The PCI device list
> will be going away sometime in the future, and we shouldn't be adding
> new ones.  Can't this just be determined from userspace instead like
> 'lspci' and 'lsusb' do?
> 
Yes, this can be done in userspace, so that stuff can be dropped.

> > +void superhyway_create_sysfs_files(struct superhyway_device *s)
> > +{
> > +	struct device *dev = &s->dev;
> > +
> > +	device_create_file(dev, &dev_attr_perr_flags);
> > +	device_create_file(dev, &dev_attr_merr_flags);
> > +	device_create_file(dev, &dev_attr_mod_vers);
> > +	device_create_file(dev, &dev_attr_mod_id);
> > +	device_create_file(dev, &dev_attr_bot_mb);
> > +	device_create_file(dev, &dev_attr_top_mb);
> > +	device_create_file(dev, &dev_attr_resource);
> 
> Can you use a default attribute list instead of manually creating the
> files individually?  Also, I don't see the code that removes these
> attributes.  Please don't rely on the sysfs function of automatically
> cleaning up the attributes when the device is removed, that might not
> work in the future.
> 
Yes, I can use the dev_attrs for this. Thanks for pointing that out.

> > +static struct superhyway_bus superhyway_bus = {
> > +	.name	= "SuperHyway bus",
> > +};
> 
> Please don't add sysfs directories that have spaces in them.  How about
> "superhyway_bus" instead?
> 
Ok.

> > +	pr_info("    0x%04x (%s) control block at 0x%08lx\n",
> > +		dev->id.id, dev->pretty_name, dev->resource.start);
> 
> Shouldn't this be a debug message?
> 
Yes, probably. Though with the devlist in userspace I suppose it's not overly
useful to have here anyways.

> > +static void __exit superhyway_bus_exit(void)
> > +{
> > +	struct superhyway_device *dev;
> > +
> > +	list_for_each_entry(dev, &superhyway_bus.devices, node) {
> > +		device_unregister(&dev->dev);
> > +		kfree(dev);
> > +	}
> 
> Ick, not good, userspace could still have a reference on the device
> through sysfs.  Don't kfree it here, do it in the release function
> (which you need to do.)
> 
Ok.

> Also, why have a local list of devices and not just use the list the
> driver core provides for you?
> 
Probably because I wasn't aware that the driver core provided one. Now that I
see the bus_for_each_xxx() stuff I'll drop the list and use that instead.

> > +struct superhyway_device {
> > +	struct list_head node;
> > +
> > +	char name[32];
> > +	char pretty_name[64];
> 
> Do you need the name and pretty_name?
> 
No, the pretty_name was only added for the devlist. I'll clean that up.

> > +struct superhyway_bus {
> > +	struct list_head devices;
> > +	struct device dev;
> > +	char name[16];
> > +};
> 
> Is the name really necessary?
> 
Probably not. With that and the list gone I can drop this struct entirely and
just have the root device as a struct device.

> > +/* drivers/sh/superhyway/superhyway-sysfs.c */
> > +#ifdef CONFIG_SYSFS
> > +void superhyway_create_sysfs_files(struct superhyway_device *);
> > +#else
> > +void superhyway_create_sysfs_files(struct superhyway_device *s)
> > +{
> > +	/* Nothing to do */
> > +}
> > +#endif
> 
> Why ifdef this function?  That should not be needed.
> 
superhyway_add_device() calls this. We don't want to assume that CONFIG_SYSFS
will always be enabled. I can move the ifdef in to superhyway_add_device() if
you think it would be cleaner. (And this is needed as we happen to have
superhyway_create_sysfs_files() defined in superhyway-sysfs.c which is only
compiled in when CONFIG_SYSFS is set, and I would rather not link this in
unconditionally).

Thanks for looking at this, I'll post a cleaned up version shortly.

Attachment: pgp00000.pgp
Description: PGP signature