| Message ID | 20190811153643.12029-1-max@enpas.org |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v4] ata/pata_buddha: Probe via modalias instead of initcall | expand |
Hi all, Thank you for your reviews. I hope this patch fixes all issues that have been raised. In case I've missed something, please let me know. Unfortunately I can't test the X-Surf part, as I don't own that board. I would be grateful for extra careful review of that part. Max
Replying to my own patch with two more questions: On 08/11/2019 05:36 PM, Max Staudt wrote: > - /* allocate host */ > - host = ata_host_alloc(&z->dev, nr_ports); Actually, this is an issue even the existing pata_buddha has: ata_host_alloc() will dev_set_drvdata(dev, host) which is fine on Buddha and Catweasel, but conflicts with zorro8390's own dev_set_drvdata() on an X-Surf board. Thus, if both pata_buddha and zorro8390 are active, only one can be unloaded. The original ide/buddha driver does not have this problem as far as I can see. This should be resolved once we get around to MFD support, as Geert suggested. Shall we leave this as-is, as it's not really a change from the status quo in pata_buddha? > +static int __init pata_buddha_late_init(void) > +{ > + struct zorro_dev *z = NULL; > + > + pr_info("pata_buddha: Scanning for stand-alone IDE controllers...\n"); > + zorro_register_driver(&pata_buddha_driver); > + > + pr_info("pata_buddha: Scanning for X-Surf boards...\n"); > + while ((z = zorro_find_device(ZORRO_PROD_INDIVIDUAL_COMPUTERS_X_SURF, z))) { > + static struct zorro_device_id xsurf_ent = > + { ZORRO_PROD_INDIVIDUAL_COMPUTERS_X_SURF, BOARD_XSURF}; > + > + pata_buddha_probe(z, &xsurf_ent); > + } > + > + return 0; > +} This is suboptimal, as we don't release memory in case pata_buddha_probe() fails. Any suggestions? > +static void __exit pata_buddha_exit(void) > +{ > + struct zorro_dev *z = NULL; > + > + pr_info("pata_buddha: Releasing X-Surf boards...\n"); > + while ((z = zorro_find_device(ZORRO_PROD_INDIVIDUAL_COMPUTERS_X_SURF, z))) { > + struct ata_host *host = dev_get_drvdata(&z->dev); > + > + if (host) > + ata_host_detach(host); > + } I guess that here we also need to manually release the resources we allocated with devm_* above. Any ideas? Thanks Max
On 8/11/19 9:28 PM, Max Staudt wrote: > Replying to my own patch with two more questions: > > > On 08/11/2019 05:36 PM, Max Staudt wrote: >> - /* allocate host */ >> - host = ata_host_alloc(&z->dev, nr_ports); > > Actually, this is an issue even the existing pata_buddha has: ata_host_alloc()> will dev_set_drvdata(dev, host) which is fine on Buddha and Catweasel, bu> conflicts with zorro8390's own dev_set_drvdata() on an X-Surf board. Thus, > if both pata_buddha and zorro8390 are active, only one can be unloaded. The > original ide/buddha driver does not have this problem as far as I can see. ide/buddha driver cannot be unloaded currently (it lacks module_exit()). > This should be resolved once we get around to MFD support, as Geert suggested. > > Shall we leave this as-is, as it's not really a change from the status quo in > pata_buddha? pata_buddha also cannot be unloaded currently (also lacks module_exit()), I think that we should leave it as it is until MFD support is added. >> +static int __init pata_buddha_late_init(void) >> +{ >> + struct zorro_dev *z = NULL; >> + >> + pr_info("pata_buddha: Scanning for stand-alone IDE controllers...\n"); >> + zorro_register_driver(&pata_buddha_driver); >> + >> + pr_info("pata_buddha: Scanning for X-Surf boards...\n"); >> + while ((z = zorro_find_device(ZORRO_PROD_INDIVIDUAL_COMPUTERS_X_SURF, z))) { >> + static struct zorro_device_id xsurf_ent = >> + { ZORRO_PROD_INDIVIDUAL_COMPUTERS_X_SURF, BOARD_XSURF}; >> + >> + pata_buddha_probe(z, &xsurf_ent); >> + } >> + >> + return 0; >> +} > > This is suboptimal, as we don't release memory in case pata_buddha_probe() > fails. Any suggestions? It should work exactly like the old code in case of X-Surf, what do we need to release? Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
Hi Bartlomiej, Thanks for your feedback! On 08/12/2019 12:42 PM, Bartlomiej Zolnierkiewicz wrote: > > ide/buddha driver cannot be unloaded currently (it lacks module_exit()). > > [... snip ...] > > It should work exactly like the old code in case of X-Surf, > what do we need to release? So what shall I do? Once an X-Surf has been detected, we refuse to unload, and therefore we never have to release X-Surf resources? That would simplify things a lot. What's a good way to do that, given that we now have module_exit() defined and an exit function is void? Thanks, Max
On 8/12/19 12:55 PM, Max Staudt wrote: > Hi Bartlomiej, > > Thanks for your feedback! Hi Max, > On 08/12/2019 12:42 PM, Bartlomiej Zolnierkiewicz wrote: >> >> ide/buddha driver cannot be unloaded currently (it lacks module_exit()). >> >> [... snip ...] >> >> It should work exactly like the old code in case of X-Surf, >> what do we need to release? > > > So what shall I do? Once an X-Surf has been detected, we refuse to > unload, and therefore we never have to release X-Surf resources? > That would simplify things a lot. Yes, it seems to be a simplest solution. > What's a good way to do that, given that we now have module_exit()> defined and an exit function is void? What about something like this: static bool xsurf_present; ... static int __init pata_buddha_late_init(void) ... if (pata_buddha_probe(z, &xsurf_ent) == 0 && xsurf_present == false) xsurf_present = true; ... static void __exit pata_buddha_exit(void) ... if (xsurf_present) return -EBUSY; ... ? Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On 08/12/2019 02:15 PM, Bartlomiej Zolnierkiewicz wrote: >> What's a good way to do that, given that we now have module_exit()> defined and an exit function is void? > > What about something like this: > > static bool xsurf_present; > ... > static int __init pata_buddha_late_init(void) > ... > if (pata_buddha_probe(z, &xsurf_ent) == 0 && > xsurf_present == false) > xsurf_present = true; > ... > static void __exit pata_buddha_exit(void) > ... > if (xsurf_present) > return -EBUSY; > ... > > ? Okay, so we're talking about the same idea. Great! Unfortunately, pata_buddha_exit() is void, and thus can't fail. According to Documentation/kernel-hacking/hacking.rst this is by design. Any other ideas? We could also continue to disallow unloading completely until MFD support comes along. Max
On 8/12/19 4:26 PM, Max Staudt wrote: > On 08/12/2019 02:15 PM, Bartlomiej Zolnierkiewicz wrote: >>> What's a good way to do that, given that we now have module_exit()> defined and an exit function is void? >> >> What about something like this: >> >> static bool xsurf_present; >> ... >> static int __init pata_buddha_late_init(void) >> ... >> if (pata_buddha_probe(z, &xsurf_ent) == 0 && >> xsurf_present == false) >> xsurf_present = true; >> ... >> static void __exit pata_buddha_exit(void) >> ... >> if (xsurf_present) >> return -EBUSY; >> ... >> >> ? > > Okay, so we're talking about the same idea. Great! > > Unfortunately, pata_buddha_exit() is void, and thus can't fail. According to Documentation/kernel-hacking/hacking.rst this is by design. You are of course right and the example code is broken (+ I need more caffeine). > Any other ideas? We could also continue to disallow unloading completely until MFD support comes along. Yes, this would also be OK. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On 08/12/2019 05:01 PM, Bartlomiej Zolnierkiewicz wrote: >> Unfortunately, pata_buddha_exit() is void, and thus can't fail. According to Documentation/kernel-hacking/hacking.rst this is by design. > > You are of course right and the example code is broken > (+ I need more caffeine). Hey! That's usually my line to say! I'm just glad I didn't miss something obvious here... >> Any other ideas? We could also continue to disallow unloading completely until MFD support comes along. > Yes, this would also be OK. Okay, I will refresh the patch accordingly. Thanks for your help! Max
diff --git a/drivers/ata/pata_buddha.c b/drivers/ata/pata_buddha.c index 11a8044ff..25d03b595 100644 --- a/drivers/ata/pata_buddha.c +++ b/drivers/ata/pata_buddha.c @@ -18,7 +18,9 @@ #include <linux/kernel.h> #include <linux/libata.h> #include <linux/mm.h> +#include <linux/mod_devicetable.h> #include <linux/module.h> +#include <linux/types.h> #include <linux/zorro.h> #include <scsi/scsi_cmnd.h> #include <scsi/scsi_host.h> @@ -29,7 +31,7 @@ #include <asm/setup.h> #define DRV_NAME "pata_buddha" -#define DRV_VERSION "0.1.0" +#define DRV_VERSION "0.1.1" #define BUDDHA_BASE1 0x800 #define BUDDHA_BASE2 0xa00 @@ -47,11 +49,11 @@ enum { BOARD_XSURF }; -static unsigned int buddha_bases[3] __initdata = { +static unsigned int buddha_bases[3] = { BUDDHA_BASE1, BUDDHA_BASE2, BUDDHA_BASE3 }; -static unsigned int xsurf_bases[2] __initdata = { +static unsigned int xsurf_bases[2] = { XSURF_BASE1, XSURF_BASE2 }; @@ -145,111 +147,166 @@ static struct ata_port_operations pata_xsurf_ops = { .set_mode = pata_buddha_set_mode, }; -static int __init pata_buddha_init_one(void) +static int pata_buddha_probe(struct zorro_dev *z, + const struct zorro_device_id *ent) { - struct zorro_dev *z = NULL; + static const char * const board_name[] + = { "Buddha", "Catweasel", "X-Surf" }; + struct ata_host *host; + void __iomem *buddha_board; + unsigned long board; + unsigned int type = ent->driver_data; + unsigned int nr_ports = (type == BOARD_CATWEASEL) ? 3 : 2; + int i; + + dev_info(&z->dev, "%s IDE controller\n", board_name[type]); + + board = z->resource.start; + + if (type != BOARD_XSURF) { + if (!devm_request_mem_region(&z->dev, + board + BUDDHA_BASE1, + 0x800, DRV_NAME)) + return -ENXIO; + } else { + if (!devm_request_mem_region(&z->dev, + board + XSURF_BASE1, + 0x1000, DRV_NAME)) + return -ENXIO; + if (!devm_request_mem_region(&z->dev, + board + XSURF_BASE2, + 0x1000, DRV_NAME)) { + } + } + + /* allocate host */ + host = ata_host_alloc(&z->dev, nr_ports); + if (!host) + return -ENXIO; + + buddha_board = ZTWO_VADDR(board); - while ((z = zorro_find_device(ZORRO_WILDCARD, z))) { - static const char *board_name[] - = { "Buddha", "Catweasel", "X-Surf" }; - struct ata_host *host; - void __iomem *buddha_board; - unsigned long board; - unsigned int type, nr_ports = 2; - int i; - - if (z->id == ZORRO_PROD_INDIVIDUAL_COMPUTERS_BUDDHA) { - type = BOARD_BUDDHA; - } else if (z->id == ZORRO_PROD_INDIVIDUAL_COMPUTERS_CATWEASEL) { - type = BOARD_CATWEASEL; - nr_ports++; - } else if (z->id == ZORRO_PROD_INDIVIDUAL_COMPUTERS_X_SURF) { - type = BOARD_XSURF; - } else - continue; - - dev_info(&z->dev, "%s IDE controller\n", board_name[type]); - - board = z->resource.start; + /* enable the board IRQ on Buddha/Catweasel */ + if (type != BOARD_XSURF) + z_writeb(0, buddha_board + BUDDHA_IRQ_MR); + + for (i = 0; i < nr_ports; i++) { + struct ata_port *ap = host->ports[i]; + void __iomem *base, *irqport; + unsigned long ctl = 0; if (type != BOARD_XSURF) { - if (!devm_request_mem_region(&z->dev, - board + BUDDHA_BASE1, - 0x800, DRV_NAME)) - continue; + ap->ops = &pata_buddha_ops; + base = buddha_board + buddha_bases[i]; + ctl = BUDDHA_CONTROL; + irqport = buddha_board + BUDDHA_IRQ + i * 0x40; } else { - if (!devm_request_mem_region(&z->dev, - board + XSURF_BASE1, - 0x1000, DRV_NAME)) - continue; - if (!devm_request_mem_region(&z->dev, - board + XSURF_BASE2, - 0x1000, DRV_NAME)) - continue; + ap->ops = &pata_xsurf_ops; + base = buddha_board + xsurf_bases[i]; + /* X-Surf has no CS1* (Control/AltStat) */ + irqport = buddha_board + XSURF_IRQ; } - /* allocate host */ - host = ata_host_alloc(&z->dev, nr_ports); - if (!host) - continue; - - buddha_board = ZTWO_VADDR(board); - - /* enable the board IRQ on Buddha/Catweasel */ - if (type != BOARD_XSURF) - z_writeb(0, buddha_board + BUDDHA_IRQ_MR); - - for (i = 0; i < nr_ports; i++) { - struct ata_port *ap = host->ports[i]; - void __iomem *base, *irqport; - unsigned long ctl = 0; - - if (type != BOARD_XSURF) { - ap->ops = &pata_buddha_ops; - base = buddha_board + buddha_bases[i]; - ctl = BUDDHA_CONTROL; - irqport = buddha_board + BUDDHA_IRQ + i * 0x40; - } else { - ap->ops = &pata_xsurf_ops; - base = buddha_board + xsurf_bases[i]; - /* X-Surf has no CS1* (Control/AltStat) */ - irqport = buddha_board + XSURF_IRQ; - } - - ap->pio_mask = ATA_PIO4; - ap->flags |= ATA_FLAG_SLAVE_POSS | ATA_FLAG_NO_IORDY; - - ap->ioaddr.data_addr = base; - ap->ioaddr.error_addr = base + 2 + 1 * 4; - ap->ioaddr.feature_addr = base + 2 + 1 * 4; - ap->ioaddr.nsect_addr = base + 2 + 2 * 4; - ap->ioaddr.lbal_addr = base + 2 + 3 * 4; - ap->ioaddr.lbam_addr = base + 2 + 4 * 4; - ap->ioaddr.lbah_addr = base + 2 + 5 * 4; - ap->ioaddr.device_addr = base + 2 + 6 * 4; - ap->ioaddr.status_addr = base + 2 + 7 * 4; - ap->ioaddr.command_addr = base + 2 + 7 * 4; - - if (ctl) { - ap->ioaddr.altstatus_addr = base + ctl; - ap->ioaddr.ctl_addr = base + ctl; - } - - ap->private_data = (void *)irqport; - - ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx", board, - ctl ? board + buddha_bases[i] + ctl : 0); + ap->pio_mask = ATA_PIO4; + ap->flags |= ATA_FLAG_SLAVE_POSS | ATA_FLAG_NO_IORDY; + + ap->ioaddr.data_addr = base; + ap->ioaddr.error_addr = base + 2 + 1 * 4; + ap->ioaddr.feature_addr = base + 2 + 1 * 4; + ap->ioaddr.nsect_addr = base + 2 + 2 * 4; + ap->ioaddr.lbal_addr = base + 2 + 3 * 4; + ap->ioaddr.lbam_addr = base + 2 + 4 * 4; + ap->ioaddr.lbah_addr = base + 2 + 5 * 4; + ap->ioaddr.device_addr = base + 2 + 6 * 4; + ap->ioaddr.status_addr = base + 2 + 7 * 4; + ap->ioaddr.command_addr = base + 2 + 7 * 4; + + if (ctl) { + ap->ioaddr.altstatus_addr = base + ctl; + ap->ioaddr.ctl_addr = base + ctl; } - ata_host_activate(host, IRQ_AMIGA_PORTS, ata_sff_interrupt, - IRQF_SHARED, &pata_buddha_sht); + ap->private_data = (void *)irqport; + ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx", board, + ctl ? board + buddha_bases[i] + ctl : 0); } + ata_host_activate(host, IRQ_AMIGA_PORTS, ata_sff_interrupt, + IRQF_SHARED, &pata_buddha_sht); + + return 0; } -module_init(pata_buddha_init_one); +static void pata_buddha_remove(struct zorro_dev *z) +{ + struct ata_host *host = dev_get_drvdata(&z->dev); + + ata_host_detach(host); +} + +static const struct zorro_device_id pata_buddha_zorro_tbl[] = { + { ZORRO_PROD_INDIVIDUAL_COMPUTERS_BUDDHA, BOARD_BUDDHA}, + { ZORRO_PROD_INDIVIDUAL_COMPUTERS_CATWEASEL, BOARD_CATWEASEL}, + /* { ZORRO_PROD_INDIVIDUAL_COMPUTERS_X_SURF, BOARD_XSURF}, */ + { 0 } +}; + +MODULE_DEVICE_TABLE(zorro, pata_buddha_zorro_tbl); + +static struct zorro_driver pata_buddha_driver = { + .name = "pata_buddha", + .id_table = pata_buddha_zorro_tbl, + .probe = pata_buddha_probe, + .remove = pata_buddha_remove, +}; + + + +/* + * We cannot have a modalias for X-Surf boards, as it competes with the + * zorro8390 network driver. As a stopgap measure until we have proper + * MFC support for this board, we manually attach to it late after Zorro + * has enumerated its boards. + */ +static int __init pata_buddha_late_init(void) +{ + struct zorro_dev *z = NULL; + + pr_info("pata_buddha: Scanning for stand-alone IDE controllers...\n"); + zorro_register_driver(&pata_buddha_driver); + + pr_info("pata_buddha: Scanning for X-Surf boards...\n"); + while ((z = zorro_find_device(ZORRO_PROD_INDIVIDUAL_COMPUTERS_X_SURF, z))) { + static struct zorro_device_id xsurf_ent = + { ZORRO_PROD_INDIVIDUAL_COMPUTERS_X_SURF, BOARD_XSURF}; + + pata_buddha_probe(z, &xsurf_ent); + } + + return 0; +} + +static void __exit pata_buddha_exit(void) +{ + struct zorro_dev *z = NULL; + + pr_info("pata_buddha: Releasing X-Surf boards...\n"); + while ((z = zorro_find_device(ZORRO_PROD_INDIVIDUAL_COMPUTERS_X_SURF, z))) { + struct ata_host *host = dev_get_drvdata(&z->dev); + + if (host) + ata_host_detach(host); + } + + pr_info("pata_buddha: Releasing stand-alone IDE controllers...\n"); + zorro_unregister_driver(&pata_buddha_driver); +} + +late_initcall(pata_buddha_late_init); +module_exit(pata_buddha_exit); + MODULE_AUTHOR("Bartlomiej Zolnierkiewicz"); MODULE_DESCRIPTION("low-level driver for Buddha/Catweasel/X-Surf PATA");
Up until now, the pata_buddha driver would only check for cards on initcall time. Now, the kernel will call its probe function as soon as a compatible card is detected. v4: Cleap up pata_buddha_probe() by using ent->driver_data, Support X-Surf via late_initcall() v3: Clean up devm_*, implement device removal. v2: Rename 'zdev' to 'z' to make the patch easy to analyse with git diff --ignore-space-change Tested-by: Max Staudt <max@enpas.org> Signed-off-by: Max Staudt <max@enpas.org> --- drivers/ata/pata_buddha.c | 243 ++++++++++++++++++++++++++++------------------ 1 file changed, 150 insertions(+), 93 deletions(-)