| Message ID | 201004271518.13611.nschichan@freebox.fr |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Apr 27, 2010 at 03:18:13PM +0200, Nicolas Schichan wrote: > On Tuesday 27 April 2010 05:42:10 am Greg KH wrote: > > On Mon, Apr 26, 2010 at 06:29:04PM +0200, Nicolas Schichan wrote: > > > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > > > index 91fed3c..179abad 100644 > > > --- a/drivers/ata/libata-core.c > > > +++ b/drivers/ata/libata-core.c > > > @@ -5660,6 +5660,10 @@ int sata_link_init_spd(struct ata_link *link) > > > return 0; > > > } > > > > > > +static void ata_port_dev_release(struct device *dev) > > > +{ > > > +} > > > > {sigh} > > > > By doing the above, you have guaranteed that I will make fun of this > > code. Consider this the ridicule it deserves. > > > > (hint, read the kobject documentation for why I get to make fun of > > it...) > > > > Think about why you created an empty release function, to try to get the > > kernel to stop spitting out a message to you. > > That's right, shame on me for not reading the documentation. > > > Didn't you think that the > > message was there for a reason, and it was not to be worked around? > > Well after reading the kobject documentation, I understand why it is > bad thing to have this function empty. Since someone may still hold a > reference on the device when I call device_unregister(), I guess the > only safe place where to kfree the struct ata_port is in the release > callback of the device. > > Please find an updated patch addressing your comments below: Looks better, thanks. As for the whole idea of the extra device (which it doesn't look like you ever initialize anywhere), it's not good to have one sitting in the middle of the device chain that isn't owned by a bus somehow. That just looks wierd and can cause problems with udev rules. why is this really needed at all? Can't you just export the port number in the device as an attribute instead? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2010-04-27 at 22:50 -0700, Greg KH wrote: Hi Greg, > As for the whole idea of the extra device (which it doesn't look like > you ever initialize anywhere), it's not good to have one sitting in the > middle of the device chain that isn't owned by a bus somehow. That just > looks wierd and can cause problems with udev rules. > > why is this really needed at all? Can't you just export the port number > in the device as an attribute instead? You're right, please disregard this. We got fooled by the incrementing scsi host id in the device path. The misleading part (for me) is that even if a device sysfs devpath *seems* to represent its topology, some parts of the path are not "persistent" (across simple rmmod/modprobe) I just read udev path_id source code and saw the trick for the scsi host number (which would have been broken with this patch). Since the ata code always creates scsi hosts in ata port order, we can assume that lowest scsi host number is the first ata port, so the patch is not needed.
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 91fed3c..f96d8c0 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -5660,6 +5660,14 @@ int sata_link_init_spd(struct ata_link *link) return 0; } +static void ata_port_dev_release(struct device *dev) +{ + struct ata_port *ap; + + ap = container_of(dev, struct ata_port, ata_port_device); + kfree(ap); +} + /** * ata_port_alloc - allocate and initialize basic ATA port resources * @host: ATA host this allocated port belongs to @@ -5672,7 +5680,7 @@ int sata_link_init_spd(struct ata_link *link) * LOCKING: * Inherited from calling layer (may sleep). */ -struct ata_port *ata_port_alloc(struct ata_host *host) +struct ata_port *ata_port_alloc(struct ata_host *host, int port_no) { struct ata_port *ap; @@ -5690,6 +5698,20 @@ struct ata_port *ata_port_alloc(struct ata_host *host) ap->host = host; ap->dev = host->dev; ap->last_ctl = 0xFF; + ap->port_no = port_no; + + /* + * register device used to add the scsi host behind this + * port. make it a parent of the struct device of the host. + */ + ap->ata_port_device.release = ata_port_dev_release; + dev_set_name(&ap->ata_port_device, "port%i", ap->port_no); + ap->ata_port_device.parent = ap->dev; + if (device_register(&ap->ata_port_device) < 0) { + kfree(ap); + return NULL; + } + #if defined(ATA_VERBOSE_DEBUG) /* turn on all debugging levels */ @@ -5741,7 +5763,11 @@ static void ata_host_release(struct device *gendev, void *res) kfree(ap->pmp_link); kfree(ap->slave_link); - kfree(ap); + + /* + * ap will be kfree'ed in device release callback. + */ + device_unregister(&ap->ata_port_device); host->ports[i] = NULL; } @@ -5797,11 +5823,10 @@ struct ata_host *ata_host_alloc(struct device *dev, int max_ports) for (i = 0; i < max_ports; i++) { struct ata_port *ap; - ap = ata_port_alloc(host); + ap = ata_port_alloc(host, i); if (!ap) goto err_out; - ap->port_no = i; host->ports[i] = ap; } diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index b4ee28d..1cbda75 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -3210,7 +3210,7 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht) */ shost->max_host_blocked = 1; - rc = scsi_add_host(ap->scsi_host, ap->host->dev); + rc = scsi_add_host(ap->scsi_host, &ap->ata_port_device); if (rc) goto err_add; } @@ -3593,11 +3593,10 @@ struct ata_port *ata_sas_port_alloc(struct ata_host *host, { struct ata_port *ap; - ap = ata_port_alloc(host); + ap = ata_port_alloc(host, 0); if (!ap) return NULL; - ap->port_no = 0; ap->lock = shost->host_lock; ap->pio_mask = port_info->pio_mask; ap->mwdma_mask = port_info->mwdma_mask; diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h index 823e630..cd44e9e 100644 --- a/drivers/ata/libata.h +++ b/drivers/ata/libata.h @@ -112,7 +112,7 @@ extern void ata_link_init(struct ata_port *ap, struct ata_link *link, int pmp); extern int sata_link_init_spd(struct ata_link *link); extern int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg); extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg); -extern struct ata_port *ata_port_alloc(struct ata_host *host); +extern struct ata_port *ata_port_alloc(struct ata_host *host, int port_no); extern void ata_dev_enable_pm(struct ata_device *dev, enum link_pm policy); extern void ata_lpm_schedule(struct ata_port *ap, enum link_pm); diff --git a/include/linux/libata.h b/include/linux/libata.h index b0f6d97..64d94d9 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -749,6 +749,9 @@ struct ata_port { struct ata_host *host; struct device *dev; + /* used as base to create scsi host behind the port.*/ + struct device ata_port_device; + void *port_task_data; struct delayed_work port_task; struct delayed_work hotplug_task;