Re: [PATCH v4] ata/pata_buddha: Probe via modalias instead of initcall
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
Re: [PATCH v4] ata/pata_buddha: Probe via modalias instead of initcall
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, _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 Institute Poland Samsung Electronics
Re: [PATCH v4] ata/pata_buddha: Probe via modalias instead of initcall
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, _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
Re: [PATCH v4] ata/pata_buddha: Probe via modalias instead of initcall
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, _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 Institute Poland Samsung Electronics
Re: [PATCH v4] ata/pata_buddha: Probe via modalias instead of initcall
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
Re: [PATCH v4] ata/pata_buddha: Probe via modalias instead of initcall
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(>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(_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, _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 Institute Poland
Samsung Electronics
Re: [PATCH v4] ata/pata_buddha: Probe via modalias instead of initcall
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(>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(_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, _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(>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
Re: [PATCH v4] ata/pata_buddha: Probe via modalias instead of initcall
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
[PATCH v4] ata/pata_buddha: Probe via modalias instead of initcall
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
Signed-off-by: Max Staudt
---
drivers/ata/pata_buddha.c | 243 --
1 file changed, 150 insertions(+), 93 deletions(-)
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
#include
#include
+#include
#include
+#include
#include
#include
#include
@@ -29,7 +31,7 @@
#include
#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(>dev, "%s IDE controller\n", board_name[type]);
+
+ board = z->resource.start;
+
+ if (type != BOARD_XSURF) {
+ if (!devm_request_mem_region(>dev,
+board + BUDDHA_BASE1,
+0x800, DRV_NAME))
+ return -ENXIO;
+ } else {
+ if (!devm_request_mem_region(>dev,
+board + XSURF_BASE1,
+0x1000, DRV_NAME))
+ return -ENXIO;
+ if (!devm_request_mem_region(>dev,
+board + XSURF_BASE2,
+0x1000, DRV_NAME)) {
+ }
+ }
+
+ /* allocate host */
+ host = ata_host_alloc(>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(>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(>dev,
-board + BUDDHA_BASE1,
-0x800, DRV_NAME))
- continue;
+ ap->ops = _buddha_ops;
+ base = buddha_board + buddha_bases[i];
+ ctl = BUDDHA_CONTROL;
+ irqport = buddha_board + BUDDHA_IRQ + i * 0x40;
} else {
- if (!devm_request_mem_region(>dev,
-board + XSURF_BASE1,
-
