Re: [PATCH] Replace pcihpfs with sysfs.

Greg KH
2003-01-23T04:47:26Z

On Wed, Jan 22, 2003 at 09:39:01AM +0800, Stanley Wang wrote:
> Hi, Grep

"Grep"? Who's that? :)

> Here is the patch that replace pcihpfs with sysfs. (against the lastest
> BK tree)

Thanks a lot for the patch, I appreciate. I do have a few
comments/questions below (they are still true for your updated patch you
sent.

> diff -Nru a/drivers/hotplug/cpci_hotplug_core.c b/drivers/hotplug/cpci_hotplug_core.c
> --- a/drivers/hotplug/cpci_hotplug_core.c Wed Jan 22 09:30:20 2003
> +++ b/drivers/hotplug/cpci_hotplug_core.c Wed Jan 22 09:30:20 2003
> @@ -130,7 +130,7 @@
> return -EINVAL;
> memcpy(&info, hotplug_slot->info, sizeof(struct hotplug_slot_info));
> info.latch_status = value;
> - return pci_hp_change_slot_info(hotplug_slot->name, &info);
> + return 0;

We really need to keep this functionality. Unfortunatly sysfs doesn't
support that yet. I'd keep the call to pci_hp_change_slot_info() around
to point out that this needs to be fixed in sysfs.

> /* Random magic number */
> #define PCIHPFS_MAGIC 0x52454541

You can also remove this #define now, right?

> +static struct subsystem hotplug_slot_subsys;

You need to initialize this structure to something, and give it a name.
Otherwise all of the slots show up in the top of sysfs, right? I would
really like to see these directories show up in /sys/bus/pci/slots/ if
you can get them to go there.

> -static ssize_t power_write_file (struct file *file, const char *ubuff, size_t count, loff_t *offset)
> +static ssize_t power_write_file (struct hotplug_slot *slot, const char *buf,
> + size_t count)
> {
> - struct hotplug_slot *slot = file->private_data;
> - char *buff;
> - unsigned long lpower;
> - u8 power;
> + unsigned long power;
> int retval = 0;
>
> - if (*offset < 0)
> - return -EINVAL;
> - if (count == 0 || count > 16384)
> - return 0;
> - if (*offset != 0)
> - return 0;
> -
> - if (slot == NULL) {
> - dbg("slot == NULL???\n");
> - return -ENODEV;
> - }
> -
> - buff = kmalloc (count + 1, GFP_KERNEL);
> - if (!buff)
> - return -ENOMEM;
> - memset (buff, 0x00, count + 1);
> -
> - if (copy_from_user ((void *)buff, (void *)ubuff, count)) {
> - retval = -EFAULT;
> + retval = sscanf(buf, "%ld", &power);
> + if (retval != 1) {
> + err("Illegla value specified for power\n");
> + retval = -EINVAL;
> goto exit;
> }
> -
> - lpower = simple_strtoul (buff, NULL, 10);
> - power = (u8)(lpower & 0xff);
> - dbg ("power = %d\n", power);
> + dbg ("power = %ld\n", power);
>
> if (!try_module_get(slot->ops->owner)) {
> retval = -ENODEV;

Why change from simple_strtoul() to sscanf()? Is this a needed change?

> static void __exit pci_hotplug_exit (void)
> {
> - cpci_hotplug_exit();
> -
> - unregister_filesystem(&pcihpfs_type);
> -
> #ifdef CONFIG_PROC_FS
> if (slotdir)
> remove_proc_entry(slotdir_name, proc_bus_pci_dir);
> #endif
> +
> + cpci_hotplug_exit();
> + subsystem_unregister(&hotplug_slot_subsys);
> }
>
> module_init(pci_hotplug_init);

Why reorder these calls? Also, the whole slotdir_name stuff can be
ripped out, as we will not be needing that proc directory anymore.

Again, thanks a lot for the patch, I think it's really close.

greg k-h