2015-01-21 0:20 GMT+09:00 Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>: > On Tue, 20 Jan 2015, Akinobu Mita wrote: > >> 2015-01-19 23:22 GMT+09:00 Tejun Heo <tj@xxxxxxxxxx>: >> > On Mon, Jan 19, 2015 at 12:05:58AM +0900, Akinobu Mita wrote: >> >> While accessing a scsi_device, the use count of the underlying LLDD >> >> module is incremented. The module reference is retrieved through >> >> .module field of struct scsi_host_template. >> >> >> >> This mapping between scsi_device and underlying LLDD module works well >> >> except some drivers which consist with the core driver and the actual >> >> LLDDs and scsi_host_template is defined in the core driver. In these >> >> cases, the actual LLDDs can be unloaded even if the scsi_device is >> >> being accessed. >> >> >> >> This patch series fixes the module reference mismatch problem for >> >> ufs, usb-storage, esp_scsi, ahci_platform, and pata_platform drivers >> >> by moving owner module reference field from struct scsi_host_template >> >> to struct Scsi_Host and allowing the LLDDs to set their correct module >> >> reference. >> > >> > Hmmm, can't we just move sht definitions to actual LLDDs? libata can >> > do that easily. sht, as its name implies, is the template for >> > creating the scsi_hosts of a given type. We're now just moving module >> > ownership from sht definition site to whatever callsite the actual >> > instance is being created which can also be wrapped in a separate >> > layer requiring explicit propagation. Why not just propagate sht's >> > directly? What's the difference? >> >> The reason I didn't move sht from the core driver to the LLDDs for >> fixing ufs and ums-* in the first place is to avoid exporting many >> symbols for callbacks in sht. But I realized that we can do it >> without that many exported symbols by creating a single function that >> returns a kmemdup()ed sht with a few change including ->module. >> >> So there are three options we can take for fixing this problem. >> I would like to know the opinions which one should be taken. >> >> (1) Add owner module field to Scsi_Host for allowing LLDDs to adjust >> it after scsi host allocation. This approach is used by v1,2,3 of >> this patch series and the scsi midlayer change is minimum. >> >> (2) Move owner module field from scsi_host_template to Scsi_Host. >> The owner module reference is retrieved from the callsite of >> scsi_host_alloc() by passing THIS_MODULE to the extra argument. >> The scsi midlayer change is small, but we need the same macro trick >> for each scsi_host_alloc() wrapper and these changes are relatively >> large (required by libata, libiscsi, libfc, cxgbi, 53c700, legacy). >> This approach is used by v4 of this patch series. >> >> (3) Allocate scsi host template for each module. No scsi midlayer >> change is required. Instead of sharing a single scsi host template >> defined in the core, create a single function that returns a >> kmemdup()ed sht with a few change including ->module so that the sub >> modules can use it as their sht. > > (3) means duplicating a reasonably large data structure in order to > alter just one field. It also means changing all the subdrivers to > make them call the new function. > > (1) is the simplest. Since the use of subdrivers in general tends to > be a special case (most SCSI drivers don't do it), I prefer to keep the > code optimized for it. In other words, I prefer option (1). If people > think (2) is better, it can always be layered on top of (1). OK, I agree that (1) is the simplest. Christoph, Option (2) required a lot of changes as this v4 shows that scsi_host_alloc() is used by several libraries on scsi mid level in various forms. It is a bit different from pci_register_driver() or similar registration interfaces. Although they also use the same macro trick to get a module reference, but they are called directly or through thin and straightforward wrapper interfaces by lower level drivers. So, option (1) seems to be a better choice than (2), (3) for fixing ufs, ums-*, and esp_scsi drivers. For ahci_platform and pata_platform, they can be fixed easily by moving sht definision to LLDDs as Tejun suggested. Do you think it is ok to prepare v5 of series in this way? -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
