From: Russell King - ARM Linux <linux@arm.linux.org.uk>
To: Eric Anholt <eric@anholt.net>
Cc: jason@lakedaemon.net, linux-kernel@vger.kernel.org,
"Noralf Trønnes" <noralf@tronnes.org>,
linux-rpi-kernel@lists.infradead.org, tglx@linutronix.de,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] irqchip: bcm2835: Add FIQ support
Date: Wed, 16 Sep 2015 20:13:15 +0100 [thread overview]
Message-ID: <20150916191314.GI21084@n2100.arm.linux.org.uk> (raw)
In-Reply-To: <20150916162157.GB21084@n2100.arm.linux.org.uk>
On Wed, Sep 16, 2015 at 05:21:57PM +0100, Russell King - ARM Linux wrote:
> On Wed, Sep 16, 2015 at 10:02:32AM -0400, Eric Anholt wrote:
> > Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> >
> > > On Mon, Sep 14, 2015 at 07:33:09AM -0700, Eric Anholt wrote:
> > >> So, while FIQ isn't used in upstream, I think it's worthwhile to merge.
> > >> It is another step to bringing the downstream developers into the fold.
> > >
> > > I want to see the code _first_. Until then, I'm sorry, this patch can't
> > > go in.
> >
> > If you just want to see "Yes, GPL-compatible code using this is
> > available", then that is:
>
> It's got nothing to do with "GPL-compatible code". I want to audit
> _all_ code that makes use of FIQ. It disgusts me that you're trying
> to dress this up as a licensing issue. It isn't.
>
> > https://github.com/raspberrypi/linux/blob/rpi-4.1.y/drivers/usb/host/dwc_otg/dwc_otg_fiq_stub.S
> > https://github.com/raspberrypi/linux/blob/rpi-4.1.y/drivers/usb/host/dwc_otg/dwc_otg_fiq_fsm.c
Some comments on the FIQ aspects of the code I find in
https://github.com/raspberrypi/linux/blob/rpi-4.1.y/drivers/usb/host/dwc_otg/
as a whole:
* For non-FIQ taking of your special FIQ lock, please add a couple of
functions called something like fiq_fsm_spin_lock_disable() and
fiq_fsm_spin_unlock_enable() which wraps up this detail.
* Please remove the "local_fiq_enable();" at the end of hcd_init_fiq() -
FIQs should be enabled already (please check, and if not, please say
so, because that's a bug.)
* You run the FIQ on CPU1 when there's two CPUs present - I hope you
conditionalise this on CPU hotplug support being disabled, or have
a means to deal with CPU1 being taken offline while the driver is
operational.
* It's good that you run set_fiq_regs() from a smp-called-function,
where we're guaranteed to be non-preemptible; I've a couple of patches
which cause set_fiq_regs() to complain if called in a preemptible
context, and the good news is that this driver won't be affected by
that change.
* The comments in dwc_otg_fiq_fsm.c are incorrect; you now have locking
so the bit about requiring FIQ and SGI on the same CPU seems to no
longer apply - or does it? However, even if they're running on the
same core, I'm completely unconvinced that the comment has ever been
correct - the SGI can be interrupted by the FIQ at any moment.
* AAPCS requires stacks to be aligned to 64-bits:
regs.ARM_sp = (long) dwc_otg_hcd->fiq_stack + (sizeof(struct fiq_stack) - 4);
where:
struct fiq_stack {
int magic1;
uint8_t stack[2048];
int magic2;
};
does not provide for that. It's also an incredibly inefficient size
to allocate - do you absolutely need 2048 bytes or will 2036 bytes do?
(Also note that C99 types are frowned upon in the kernel.)
So, I'd suggest:
struct fiq_stack {
u32 magic1;
u32 stack[2040 / sizeof(u32)];
u32 magic2;
};
and:
/* Get top of stack, which must be 64-bit aligned */
regs.ARM_sp = (long)&dwc_otg_hcd->fiq_stack->magic2;
WARN_ON(regs.ARM_sp & 7);
I think that's all the comments I have on the FIQ implementation there.
What are the plans to get some of this USB code posted for review and
potential merging?
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
WARNING: multiple messages have this Message-ID (diff)
From: linux@arm.linux.org.uk (Russell King - ARM Linux)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] irqchip: bcm2835: Add FIQ support
Date: Wed, 16 Sep 2015 20:13:15 +0100 [thread overview]
Message-ID: <20150916191314.GI21084@n2100.arm.linux.org.uk> (raw)
In-Reply-To: <20150916162157.GB21084@n2100.arm.linux.org.uk>
On Wed, Sep 16, 2015 at 05:21:57PM +0100, Russell King - ARM Linux wrote:
> On Wed, Sep 16, 2015 at 10:02:32AM -0400, Eric Anholt wrote:
> > Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> >
> > > On Mon, Sep 14, 2015 at 07:33:09AM -0700, Eric Anholt wrote:
> > >> So, while FIQ isn't used in upstream, I think it's worthwhile to merge.
> > >> It is another step to bringing the downstream developers into the fold.
> > >
> > > I want to see the code _first_. Until then, I'm sorry, this patch can't
> > > go in.
> >
> > If you just want to see "Yes, GPL-compatible code using this is
> > available", then that is:
>
> It's got nothing to do with "GPL-compatible code". I want to audit
> _all_ code that makes use of FIQ. It disgusts me that you're trying
> to dress this up as a licensing issue. It isn't.
>
> > https://github.com/raspberrypi/linux/blob/rpi-4.1.y/drivers/usb/host/dwc_otg/dwc_otg_fiq_stub.S
> > https://github.com/raspberrypi/linux/blob/rpi-4.1.y/drivers/usb/host/dwc_otg/dwc_otg_fiq_fsm.c
Some comments on the FIQ aspects of the code I find in
https://github.com/raspberrypi/linux/blob/rpi-4.1.y/drivers/usb/host/dwc_otg/
as a whole:
* For non-FIQ taking of your special FIQ lock, please add a couple of
functions called something like fiq_fsm_spin_lock_disable() and
fiq_fsm_spin_unlock_enable() which wraps up this detail.
* Please remove the "local_fiq_enable();" at the end of hcd_init_fiq() -
FIQs should be enabled already (please check, and if not, please say
so, because that's a bug.)
* You run the FIQ on CPU1 when there's two CPUs present - I hope you
conditionalise this on CPU hotplug support being disabled, or have
a means to deal with CPU1 being taken offline while the driver is
operational.
* It's good that you run set_fiq_regs() from a smp-called-function,
where we're guaranteed to be non-preemptible; I've a couple of patches
which cause set_fiq_regs() to complain if called in a preemptible
context, and the good news is that this driver won't be affected by
that change.
* The comments in dwc_otg_fiq_fsm.c are incorrect; you now have locking
so the bit about requiring FIQ and SGI on the same CPU seems to no
longer apply - or does it? However, even if they're running on the
same core, I'm completely unconvinced that the comment has ever been
correct - the SGI can be interrupted by the FIQ at any moment.
* AAPCS requires stacks to be aligned to 64-bits:
regs.ARM_sp = (long) dwc_otg_hcd->fiq_stack + (sizeof(struct fiq_stack) - 4);
where:
struct fiq_stack {
int magic1;
uint8_t stack[2048];
int magic2;
};
does not provide for that. It's also an incredibly inefficient size
to allocate - do you absolutely need 2048 bytes or will 2036 bytes do?
(Also note that C99 types are frowned upon in the kernel.)
So, I'd suggest:
struct fiq_stack {
u32 magic1;
u32 stack[2040 / sizeof(u32)];
u32 magic2;
};
and:
/* Get top of stack, which must be 64-bit aligned */
regs.ARM_sp = (long)&dwc_otg_hcd->fiq_stack->magic2;
WARN_ON(regs.ARM_sp & 7);
I think that's all the comments I have on the FIQ implementation there.
What are the plans to get some of this USB code posted for review and
potential merging?
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
next prev parent reply other threads:[~2015-09-16 19:13 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-12 17:26 [PATCH] irqchip: bcm2835: Add FIQ support Noralf Trønnes
2015-06-12 17:26 ` Noralf Trønnes
2015-06-18 2:26 ` Stephen Warren
2015-06-18 2:26 ` Stephen Warren
2015-06-18 13:32 ` Noralf Trønnes
2015-06-18 13:32 ` Noralf Trønnes
2015-06-18 16:23 ` Noralf Trønnes
2015-06-18 16:23 ` Noralf Trønnes
2015-07-11 4:09 ` Stephen Warren
2015-07-11 4:09 ` Stephen Warren
2015-07-11 15:26 ` Noralf Trønnes
2015-07-11 15:26 ` Noralf Trønnes
2015-07-14 4:50 ` Stephen Warren
2015-07-14 4:50 ` Stephen Warren
2015-07-14 11:48 ` Noralf Trønnes
2015-07-14 11:48 ` Noralf Trønnes
2015-07-22 1:50 ` Stephen Warren
2015-07-22 1:50 ` Stephen Warren
2015-07-22 14:07 ` Noralf Trønnes
2015-07-22 14:07 ` Noralf Trønnes
2015-07-24 4:04 ` Stephen Warren
2015-07-24 4:04 ` Stephen Warren
2015-07-22 21:32 ` Eric Anholt
2015-07-22 21:32 ` Eric Anholt
2015-09-13 19:24 ` Noralf Trønnes
2015-09-13 19:24 ` Noralf Trønnes
2015-09-14 9:08 ` Russell King - ARM Linux
2015-09-14 9:08 ` Russell King - ARM Linux
2015-09-14 14:33 ` Eric Anholt
2015-09-14 14:33 ` Eric Anholt
2015-09-14 14:34 ` Russell King - ARM Linux
2015-09-14 14:34 ` Russell King - ARM Linux
2015-09-16 14:02 ` Eric Anholt
2015-09-16 14:02 ` Eric Anholt
2015-09-16 16:21 ` Russell King - ARM Linux
2015-09-16 16:21 ` Russell King - ARM Linux
2015-09-16 18:48 ` Eric Anholt
2015-09-16 18:48 ` Eric Anholt
2015-09-16 19:13 ` Russell King - ARM Linux [this message]
2015-09-16 19:13 ` Russell King - ARM Linux
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150916191314.GI21084@n2100.arm.linux.org.uk \
--to=linux@arm.linux.org.uk \
--cc=eric@anholt.net \
--cc=jason@lakedaemon.net \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rpi-kernel@lists.infradead.org \
--cc=noralf@tronnes.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.