Currently the IOP message pool and queues are allocated statically,
wasting ca. 5 KiB when running a Mac or multi-platform kernel on a machine
not having a Mac IOP. Convert them to conditional dynamic memory
allocations to fix this.
As kzalloc() returns zeroed memory, there's no need to initialize (parts of)
the allocated structures to zeroes.
This required moving the call to iop_init() from mac_identify() to
mac_platform_init() (slab nor bootmem are available at mac_identify() call
time), and allowed to integrate iop_register_interrupts() into iop_init().
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
Untested due to lack of hardware
arch/m68k/include/asm/mac_iop.h | 2 --
arch/m68k/mac/config.c | 3 ++-
arch/m68k/mac/iop.c | 57 ++++++++++++++++++---------------------
arch/m68k/mac/macints.c | 1 -
4 files changed, 28 insertions(+), 35 deletions(-)
diff --git a/arch/m68k/include/asm/mac_iop.h b/arch/m68k/include/asm/mac_iop.h
index fde874a..a2c7e6f 100644
--- a/arch/m68k/include/asm/mac_iop.h
+++ b/arch/m68k/include/asm/mac_iop.h
@@ -159,6 +159,4 @@ extern void iop_upload_code(uint, __u8 *, uint, __u16);
extern void iop_download_code(uint, __u8 *, uint, __u16);
extern __u8 *iop_compare_code(uint, __u8 *, uint, __u16);
-extern void iop_register_interrupts(void);
-
#endif /* __ASSEMBLY__ */
diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
index afb95d5..a8d62a8 100644
--- a/arch/m68k/mac/config.c
+++ b/arch/m68k/mac/config.c
@@ -925,7 +925,6 @@ static void __init mac_identify(void)
printk(KERN_DEBUG " Machine ID: %ld CPUid: 0x%lx memory size: 0x%lx\n",
mac_bi_data.id, mac_bi_data.cpuid, mac_bi_data.memsize);
- iop_init();
via_init();
oss_init();
psc_init();
@@ -983,6 +982,8 @@ int __init mac_platform_init(void)
if (!MACH_IS_MAC)
return -ENODEV;
+ iop_init();
+
/*
* Serial devices
*/
diff --git a/arch/m68k/mac/iop.c b/arch/m68k/mac/iop.c
index 8a4c446..6cc3055 100644
--- a/arch/m68k/mac/iop.c
+++ b/arch/m68k/mac/iop.c
@@ -104,6 +104,7 @@
* should execute quickly.)
*/
+#include <linux/slab.h>
#include <linux/types.h>
#include <linux/kernel.h>
#include <linux/mm.h>
@@ -142,9 +143,9 @@ static volatile struct mac_iop *iop_base[NUM_IOPS];
* IOP message queues
*/
-static struct iop_msg iop_msg_pool[NUM_IOP_MSGS];
-static struct iop_msg *iop_send_queue[NUM_IOPS][NUM_IOP_CHAN];
-static struct listener iop_listeners[NUM_IOPS][NUM_IOP_CHAN];
+static struct iop_msg *iop_msg_pool;
+static struct iop_msg **iop_send_queue[NUM_IOPS];
+static struct listener *iop_listeners[NUM_IOPS];
irqreturn_t iop_ism_irq(int, void *);
@@ -260,47 +261,38 @@ void __init iop_preinit(void)
}
}
+static void __init alloc_msg_queue(int iop_num)
+{
+ iop_send_queue[iop_num] =
+ kzalloc(NUM_IOP_CHAN * sizeof(**iop_send_queue), GFP_KERNEL);
+ iop_listeners[iop_num] =
+ kzalloc(NUM_IOP_CHAN * sizeof(**iop_listeners), GFP_KERNEL);
+}
+
/*
* Initialize the IOPs, if present.
*/
void __init iop_init(void)
{
- int i;
+ if (!iop_scc_present && !iop_ism_present)
+ return;
+
+ iop_msg_pool = kzalloc(NUM_IOP_MSGS * sizeof(*iop_msg_pool),
+ GFP_KERNEL);
if (iop_scc_present) {
printk("IOP: detected SCC IOP at %p\n", iop_base[IOP_NUM_SCC]);
+ alloc_msg_queue(IOP_NUM_SCC);
}
if (iop_ism_present) {
printk("IOP: detected ISM IOP at %p\n", iop_base[IOP_NUM_ISM]);
- iop_start(iop_base[IOP_NUM_ISM]);
- iop_alive(iop_base[IOP_NUM_ISM]); /* clears the alive flag */
- }
-
- /* Make the whole pool available and empty the queues */
-
- for (i = 0 ; i < NUM_IOP_MSGS ; i++) {
- iop_msg_pool[i].status = IOP_MSGSTATUS_UNUSED;
- }
+ alloc_msg_queue(IOP_NUM_ISM);
- for (i = 0 ; i < NUM_IOP_CHAN ; i++) {
- iop_send_queue[IOP_NUM_SCC][i] = NULL;
- iop_send_queue[IOP_NUM_ISM][i] = NULL;
- iop_listeners[IOP_NUM_SCC][i].devname = NULL;
- iop_listeners[IOP_NUM_SCC][i].handler = NULL;
- iop_listeners[IOP_NUM_ISM][i].devname = NULL;
- iop_listeners[IOP_NUM_ISM][i].handler = NULL;
- }
-}
-
-/*
- * Register the interrupt handler for the IOPs.
- * TODO: might be wrong for non-OSS machines. Anyone?
- */
-
-void __init iop_register_interrupts(void)
-{
- if (iop_ism_present) {
+ /*
+ * Register the interrupt handler for the IOPs.
+ * TODO: might be wrong for non-OSS machines. Anyone?
+ */
if (macintosh_config->ident == MAC_MODEL_IIFX) {
if (request_irq(IRQ_MAC_ADB, iop_ism_irq, 0,
"ISM IOP", (void *)IOP_NUM_ISM))
@@ -315,6 +307,9 @@ void __init iop_register_interrupts(void)
} else {
printk("IOP: the ISM IOP seems to be alive.\n");
}
+
+ iop_start(iop_base[IOP_NUM_ISM]);
+ iop_alive(iop_base[IOP_NUM_ISM]); /* clears the alive flag */
}
}
diff --git a/arch/m68k/mac/macints.c b/arch/m68k/mac/macints.c
index 5c1a6b2..da42c18 100644
--- a/arch/m68k/mac/macints.c
+++ b/arch/m68k/mac/macints.c
@@ -178,7 +178,6 @@ void __init mac_init_IRQ(void)
psc_register_interrupts();
if (baboon_present)
baboon_register_interrupts();
- iop_register_interrupts();
if (request_irq(IRQ_AUTO_7, mac_nmi_handler, 0, "NMI",
mac_nmi_handler))
pr_err("Couldn't register NMI\n");
--
1.7.9.5
On Sun, Jun 30, 2013 at 12:02:22PM +0200, Geert Uytterhoeven wrote:
> Currently the IOP message pool and queues are allocated statically,
> wasting ca. 5 KiB when running a Mac or multi-platform kernel on a machine
> not having a Mac IOP. Convert them to conditional dynamic memory
> allocations to fix this.
>
> As kzalloc() returns zeroed memory, there's no need to initialize (parts of)
> the allocated structures to zeroes.
>
> This required moving the call to iop_init() from mac_identify() to
> mac_platform_init() (slab nor bootmem are available at mac_identify() call
> time), and allowed to integrate iop_register_interrupts() into iop_init().
Just a couple very minor comments. I haven't tested it, but I was involved
with the original implementation and testing of this code.
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> Untested due to lack of hardware
>
> arch/m68k/include/asm/mac_iop.h | 2 --
> arch/m68k/mac/config.c | 3 ++-
> arch/m68k/mac/iop.c | 57 ++++++++++++++++++---------------------
> arch/m68k/mac/macints.c | 1 -
> 4 files changed, 28 insertions(+), 35 deletions(-)
>
> diff --git a/arch/m68k/include/asm/mac_iop.h b/arch/m68k/include/asm/mac_iop.h
> index fde874a..a2c7e6f 100644
> --- a/arch/m68k/include/asm/mac_iop.h
> +++ b/arch/m68k/include/asm/mac_iop.h
> @@ -159,6 +159,4 @@ extern void iop_upload_code(uint, __u8 *, uint, __u16);
> extern void iop_download_code(uint, __u8 *, uint, __u16);
> extern __u8 *iop_compare_code(uint, __u8 *, uint, __u16);
>
> -extern void iop_register_interrupts(void);
> -
> #endif /* __ASSEMBLY__ */
> diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
> index afb95d5..a8d62a8 100644
> --- a/arch/m68k/mac/config.c
> +++ b/arch/m68k/mac/config.c
> @@ -925,7 +925,6 @@ static void __init mac_identify(void)
> printk(KERN_DEBUG " Machine ID: %ld CPUid: 0x%lx memory size: 0x%lx\n",
> mac_bi_data.id, mac_bi_data.cpuid, mac_bi_data.memsize);
>
> - iop_init();
> via_init();
> oss_init();
> psc_init();
> @@ -983,6 +982,8 @@ int __init mac_platform_init(void)
> if (!MACH_IS_MAC)
> return -ENODEV;
>
> + iop_init();
> +
> /*
> * Serial devices
> */
> diff --git a/arch/m68k/mac/iop.c b/arch/m68k/mac/iop.c
> index 8a4c446..6cc3055 100644
> --- a/arch/m68k/mac/iop.c
> +++ b/arch/m68k/mac/iop.c
> @@ -104,6 +104,7 @@
> * should execute quickly.)
> */
>
> +#include <linux/slab.h>
> #include <linux/types.h>
> #include <linux/kernel.h>
> #include <linux/mm.h>
> @@ -142,9 +143,9 @@ static volatile struct mac_iop *iop_base[NUM_IOPS];
> * IOP message queues
> */
>
> -static struct iop_msg iop_msg_pool[NUM_IOP_MSGS];
> -static struct iop_msg *iop_send_queue[NUM_IOPS][NUM_IOP_CHAN];
> -static struct listener iop_listeners[NUM_IOPS][NUM_IOP_CHAN];
> +static struct iop_msg *iop_msg_pool;
> +static struct iop_msg **iop_send_queue[NUM_IOPS];
> +static struct listener *iop_listeners[NUM_IOPS];
>
> irqreturn_t iop_ism_irq(int, void *);
>
> @@ -260,47 +261,38 @@ void __init iop_preinit(void)
> }
> }
>
> +static void __init alloc_msg_queue(int iop_num)
> +{
> + iop_send_queue[iop_num] =
> + kzalloc(NUM_IOP_CHAN * sizeof(**iop_send_queue), GFP_KERNEL);
> + iop_listeners[iop_num] =
> + kzalloc(NUM_IOP_CHAN * sizeof(**iop_listeners), GFP_KERNEL);
> +}
> +
> /*
> * Initialize the IOPs, if present.
> */
>
> void __init iop_init(void)
> {
> - int i;
> + if (!iop_scc_present && !iop_ism_present)
> + return;
> +
> + iop_msg_pool = kzalloc(NUM_IOP_MSGS * sizeof(*iop_msg_pool),
> + GFP_KERNEL);
>
> if (iop_scc_present) {
> printk("IOP: detected SCC IOP at %p\n", iop_base[IOP_NUM_SCC]);
> + alloc_msg_queue(IOP_NUM_SCC);
Technically, this isn't actually useful. As long as we never start this
IOP, it can't ever send or be sent any messages. We currently leave the
SCC IOP in bypass mode and access the SCC chip directly.
Perhaps it would be better to allocate it at the beginning of iop_start
if it is not already allocated?
> }
> if (iop_ism_present) {
> printk("IOP: detected ISM IOP at %p\n", iop_base[IOP_NUM_ISM]);
> - iop_start(iop_base[IOP_NUM_ISM]);
> - iop_alive(iop_base[IOP_NUM_ISM]); /* clears the alive flag */
> - }
> -
> - /* Make the whole pool available and empty the queues */
> -
> - for (i = 0 ; i < NUM_IOP_MSGS ; i++) {
> - iop_msg_pool[i].status = IOP_MSGSTATUS_UNUSED;
> - }
> + alloc_msg_queue(IOP_NUM_ISM);
>
> - for (i = 0 ; i < NUM_IOP_CHAN ; i++) {
> - iop_send_queue[IOP_NUM_SCC][i] = NULL;
> - iop_send_queue[IOP_NUM_ISM][i] = NULL;
> - iop_listeners[IOP_NUM_SCC][i].devname = NULL;
> - iop_listeners[IOP_NUM_SCC][i].handler = NULL;
> - iop_listeners[IOP_NUM_ISM][i].devname = NULL;
> - iop_listeners[IOP_NUM_ISM][i].handler = NULL;
> - }
> -}
> -
> -/*
> - * Register the interrupt handler for the IOPs.
> - * TODO: might be wrong for non-OSS machines. Anyone?
> - */
> -
> -void __init iop_register_interrupts(void)
> -{
> - if (iop_ism_present) {
> + /*
> + * Register the interrupt handler for the IOPs.
> + * TODO: might be wrong for non-OSS machines. Anyone?
> + */
> if (macintosh_config->ident == MAC_MODEL_IIFX) {
> if (request_irq(IRQ_MAC_ADB, iop_ism_irq, 0,
> "ISM IOP", (void *)IOP_NUM_ISM))
> @@ -315,6 +307,9 @@ void __init iop_register_interrupts(void)
> } else {
> printk("IOP: the ISM IOP seems to be alive.\n");
> }
The if/else above isn't useful if it is run before the call to iop_start.
However, it's also useless if it is called immediately after the call to
iop_alive which is now below. It was supposed to eventually be called in
the background on a regular schedule, but that never happened.
> +
> + iop_start(iop_base[IOP_NUM_ISM]);
> + iop_alive(iop_base[IOP_NUM_ISM]); /* clears the alive flag */
> }
> }
>
> diff --git a/arch/m68k/mac/macints.c b/arch/m68k/mac/macints.c
> index 5c1a6b2..da42c18 100644
> --- a/arch/m68k/mac/macints.c
> +++ b/arch/m68k/mac/macints.c
> @@ -178,7 +178,6 @@ void __init mac_init_IRQ(void)
> psc_register_interrupts();
> if (baboon_present)
> baboon_register_interrupts();
> - iop_register_interrupts();
> if (request_irq(IRQ_AUTO_7, mac_nmi_handler, 0, "NMI",
> mac_nmi_handler))
> pr_err("Couldn't register NMI\n");
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 30 Jun 2013, Geert Uytterhoeven wrote:
> +static void __init alloc_msg_queue(int iop_num)
> +{
> + iop_send_queue[iop_num] =
> + kzalloc(NUM_IOP_CHAN * sizeof(**iop_send_queue), GFP_KERNEL);
> + iop_listeners[iop_num] =
> + kzalloc(NUM_IOP_CHAN * sizeof(**iop_listeners), GFP_KERNEL);
Perhaps we should panic on allocation failure? It might upset the static
checkers otherwise. Can be done in another patch I guess.
> -
> -/*
> - * Register the interrupt handler for the IOPs.
> - * TODO: might be wrong for non-OSS machines. Anyone?
> - */
> -
> -void __init iop_register_interrupts(void)
> -{
> - if (iop_ism_present) {
> + /*
> + * Register the interrupt handler for the IOPs.
> + * TODO: might be wrong for non-OSS machines. Anyone?
My testing with a non-OSS machine (Quadra 950) indicates that the TODO is
obsolete. And the comment "register the interrupt handler for the IOPs" is
a bit silly. It merely re-phrases the code. Otherwise, this patch looks
good to me. Thanks.
Finn
On Sun, 30 Jun 2013, Brad Boyer wrote:
> On Sun, Jun 30, 2013 at 12:02:22PM +0200, Geert Uytterhoeven wrote:
> >
> > if (iop_scc_present) {
> > printk("IOP: detected SCC IOP at %p\n", iop_base[IOP_NUM_SCC]);
> > + alloc_msg_queue(IOP_NUM_SCC);
>
> Technically, this isn't actually useful. As long as we never start this
> IOP, it can't ever send or be sent any messages.
That assumes that the SCC IOP is in a stopped state after iop_preinit().
But I don't think that's the case. If it is technically possible to
exchange messages with a device in bypass mode (?) then I think the code
above is correct.
> > @@ -315,6 +307,9 @@ void __init iop_register_interrupts(void)
> > } else {
> > printk("IOP: the ISM IOP seems to be alive.\n");
> > }
>
> The if/else above isn't useful if it is run before the call to
> iop_start. However, it's also useless if it is called immediately after
> the call to iop_alive which is now below. It was supposed to eventually
> be called in the background on a regular schedule, but that never
> happened.
>
I agree. That if/else statement and the conditional printk's should be
omitted.
I had thought that in the existing code the interrupt service routine was
registered before the call to iop_start() but in fact iop_start() happens
first. Looks like a bug to me. The interrupt handler needs to be able to
acknowledge unsolicited messages...
Finn
On Mon, Jul 01, 2013 at 05:46:48PM +1000, Finn Thain wrote:
>
> On Sun, 30 Jun 2013, Geert Uytterhoeven wrote:
>
> > -
> > -/*
> > - * Register the interrupt handler for the IOPs.
> > - * TODO: might be wrong for non-OSS machines. Anyone?
> > - */
> > -
> > -void __init iop_register_interrupts(void)
> > -{
> > - if (iop_ism_present) {
> > + /*
> > + * Register the interrupt handler for the IOPs.
> > + * TODO: might be wrong for non-OSS machines. Anyone?
>
> My testing with a non-OSS machine (Quadra 950) indicates that the TODO is
> obsolete. And the comment "register the interrupt handler for the IOPs" is
> a bit silly. It merely re-phrases the code. Otherwise, this patch looks
> good to me. Thanks.
The code was all originally written and tested only on the IIfx. That's
all JMT had, and at the time it was all I had as well. I have since
acquired a Quadra 950. Lots of these comments are just out of date. I
know it got tested on either a Quadra 900 or Quadra 950 after that
comment was put in the source.
Brad Boyer
[email protected]
On Mon, Jul 01, 2013 at 05:48:22PM +1000, Finn Thain wrote:
>
> On Sun, 30 Jun 2013, Brad Boyer wrote:
>
> > On Sun, Jun 30, 2013 at 12:02:22PM +0200, Geert Uytterhoeven wrote:
> > >
> > > if (iop_scc_present) {
> > > printk("IOP: detected SCC IOP at %p\n", iop_base[IOP_NUM_SCC]);
> > > + alloc_msg_queue(IOP_NUM_SCC);
> >
> > Technically, this isn't actually useful. As long as we never start this
> > IOP, it can't ever send or be sent any messages.
>
> That assumes that the SCC IOP is in a stopped state after iop_preinit().
> But I don't think that's the case. If it is technically possible to
> exchange messages with a device in bypass mode (?) then I think the code
> above is correct.
You're right. The value we write during iop_preinit actually leaves the
IOP running. I'm not sure why we did that. I wonder if it's possible to
put the ISM IOP into bypass mode and still use the ADB driver. Then we
could possibly use the normal SWIM driver on these systems.
> > > @@ -315,6 +307,9 @@ void __init iop_register_interrupts(void)
> > > } else {
> > > printk("IOP: the ISM IOP seems to be alive.\n");
> > > }
> >
> > The if/else above isn't useful if it is run before the call to
> > iop_start. However, it's also useless if it is called immediately after
> > the call to iop_alive which is now below. It was supposed to eventually
> > be called in the background on a regular schedule, but that never
> > happened.
> >
>
> I agree. That if/else statement and the conditional printk's should be
> omitted.
>
> I had thought that in the existing code the interrupt service routine was
> registered before the call to iop_start() but in fact iop_start() happens
> first. Looks like a bug to me. The interrupt handler needs to be able to
> acknowledge unsolicited messages...
Yes, we definitely should install the IRQ handler before calling iop_start.
Brad Boyer
[email protected]