[PATCH] m68k: mvme147, mvme16x: Don't wipe PCC timer config bits
Don't clear the timer 1 configuration bits when clearing the interrupt flag
and counter overflow. As Michael reported, "This results in no timer
interrupts being delivered after the first. Initialization then hangs
in calibrate_delay as the jiffies counter is not updated."
On mvme16x, enable the timer after requesting the irq, consistent with
mvme147.
Cc: Michael Pavone
Fixes: 7529b90d051e ("m68k: mvme147: Handle timer counter overflow")
Fixes: 1a8b8782 ("m68k: mvme16x: Handle timer counter overflow")
Reported-and-tested-by: Michael Pavone
Signed-off-by: Finn Thain
---
arch/m68k/include/asm/mvme147hw.h | 3 +++
arch/m68k/mvme147/config.c| 14 --
arch/m68k/mvme16x/config.c| 14 --
3 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/arch/m68k/include/asm/mvme147hw.h
b/arch/m68k/include/asm/mvme147hw.h
index 257b29184af9..e28eb1c0e0bf 100644
--- a/arch/m68k/include/asm/mvme147hw.h
+++ b/arch/m68k/include/asm/mvme147hw.h
@@ -66,6 +66,9 @@ struct pcc_regs {
#define PCC_INT_ENAB 0x08
#define PCC_TIMER_INT_CLR 0x80
+
+#define PCC_TIMER_TIC_EN 0x01
+#define PCC_TIMER_COC_EN 0x02
#define PCC_TIMER_CLR_OVF 0x04
#define PCC_LEVEL_ABORT0x07
diff --git a/arch/m68k/mvme147/config.c b/arch/m68k/mvme147/config.c
index cfdc7f912e14..e1e90c49a496 100644
--- a/arch/m68k/mvme147/config.c
+++ b/arch/m68k/mvme147/config.c
@@ -114,8 +114,10 @@ static irqreturn_t mvme147_timer_int (int irq, void
*dev_id)
unsigned long flags;
local_irq_save(flags);
- m147_pcc->t1_int_cntrl = PCC_TIMER_INT_CLR;
- m147_pcc->t1_cntrl = PCC_TIMER_CLR_OVF;
+ m147_pcc->t1_cntrl = PCC_TIMER_CLR_OVF | PCC_TIMER_COC_EN |
+PCC_TIMER_TIC_EN;
+ m147_pcc->t1_int_cntrl = PCC_INT_ENAB | PCC_TIMER_INT_CLR |
+PCC_LEVEL_TIMER1;
clk_total += PCC_TIMER_CYCLES;
legacy_timer_tick(1);
local_irq_restore(flags);
@@ -133,10 +135,10 @@ void mvme147_sched_init (void)
/* Init the clock with a value */
/* The clock counter increments until 0x then reloads */
m147_pcc->t1_preload = PCC_TIMER_PRELOAD;
- m147_pcc->t1_cntrl = 0x0; /* clear timer */
- m147_pcc->t1_cntrl = 0x3; /* start timer */
- m147_pcc->t1_int_cntrl = PCC_TIMER_INT_CLR; /* clear pending ints */
- m147_pcc->t1_int_cntrl = PCC_INT_ENAB|PCC_LEVEL_TIMER1;
+ m147_pcc->t1_cntrl = PCC_TIMER_CLR_OVF | PCC_TIMER_COC_EN |
+PCC_TIMER_TIC_EN;
+ m147_pcc->t1_int_cntrl = PCC_INT_ENAB | PCC_TIMER_INT_CLR |
+PCC_LEVEL_TIMER1;
clocksource_register_hz(_clk, PCC_TIMER_CLOCK_FREQ);
}
diff --git a/arch/m68k/mvme16x/config.c b/arch/m68k/mvme16x/config.c
index 30357fe4ba6c..b59593c7cfb9 100644
--- a/arch/m68k/mvme16x/config.c
+++ b/arch/m68k/mvme16x/config.c
@@ -366,6 +366,7 @@ static u32 clk_total;
#define PCCTOVR1_COC_EN 0x02
#define PCCTOVR1_OVR_CLR 0x04
+#define PCCTIC1_INT_LEVEL6
#define PCCTIC1_INT_CLR 0x08
#define PCCTIC1_INT_EN 0x10
@@ -374,8 +375,8 @@ static irqreturn_t mvme16x_timer_int (int irq, void *dev_id)
unsigned long flags;
local_irq_save(flags);
- out_8(PCCTIC1, in_8(PCCTIC1) | PCCTIC1_INT_CLR);
- out_8(PCCTOVR1, PCCTOVR1_OVR_CLR);
+ out_8(PCCTOVR1, PCCTOVR1_OVR_CLR | PCCTOVR1_TIC_EN | PCCTOVR1_COC_EN);
+ out_8(PCCTIC1, PCCTIC1_INT_EN | PCCTIC1_INT_CLR | PCCTIC1_INT_LEVEL);
clk_total += PCC_TIMER_CYCLES;
legacy_timer_tick(1);
local_irq_restore(flags);
@@ -389,14 +390,15 @@ void mvme16x_sched_init(void)
int irq;
/* Using PCCchip2 or MC2 chip tick timer 1 */
-out_be32(PCCTCNT1, 0);
-out_be32(PCCTCMP1, PCC_TIMER_CYCLES);
-out_8(PCCTOVR1, in_8(PCCTOVR1) | PCCTOVR1_TIC_EN | PCCTOVR1_COC_EN);
-out_8(PCCTIC1, PCCTIC1_INT_EN | 6);
if (request_irq(MVME16x_IRQ_TIMER, mvme16x_timer_int, IRQF_TIMER, "timer",
NULL))
panic ("Couldn't register timer int");
+out_be32(PCCTCNT1, 0);
+out_be32(PCCTCMP1, PCC_TIMER_CYCLES);
+out_8(PCCTOVR1, PCCTOVR1_OVR_CLR | PCCTOVR1_TIC_EN | PCCTOVR1_COC_EN);
+out_8(PCCTIC1, PCCTIC1_INT_EN | PCCTIC1_INT_CLR | PCCTIC1_INT_LEVEL);
+
clocksource_register_hz(_clk, PCC_TIMER_CLOCK_FREQ);
if (brdno == 0x0162 || brdno == 0x172)
--
2.26.2
Re: remove the legacy ide driver
On Thu, 18 Mar 2021, Christoph Hellwig wrote: > Hi all, > > we've been trying to get rid of the legacy ide driver for a while now, > and finally scheduled a removal for 2021, which is three month old now. > > In general distros and most defconfigs have switched to libata long ago, > but there are a few exceptions. This series first switches over all > remaining defconfigs to use libata and then removes the legacy ide > driver. > > libata mostly covers all hardware supported by the legacy ide driver. > There are three mips drivers that are not supported, but the linux-mips > list could not identify any users of those. There also are two m68k > drivers that do not have libata equivalents, which might or might not > have users, so we'll need some input and possibly help from the m68k > community here. > A few months ago I wrote another patch to move some more platforms away from macide but it has not been tested yet. That is not to say you should wait. However, my patch does have some changes that are missing from your patch series, relating to ide platform devices in arch/m68k/mac/config.c. I hope to be able to test this patch before the 5.13 merge window closes.
Re: [PATCH RESEND] ide/falconide: Fix module unload
On Sun, 3 Jan 2021, Jens Axboe wrote: > > > > This patch was sent in September and subsequently resent in November. > > I've since learned that the maintainer has been ill. What's the best > > way forward for fixes like this? > > I can queue it up. > That would be great.
RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers
On Wed, 24 Feb 2021, Song Bao Hua (Barry Song) wrote: > > Realtime requirement is definitely a true requirement on ARM Linux. > > I once talked/worked with some guys who were using ARM for realtime > system. > The feasible approaches include: > 1. Dual OS(RTOS + Linux): e.g. QNX+Linux XENOMAI+Linux L4+Linux > 2. preempt-rt > Which is continuously maintained like: > https://lore.kernel.org/lkml/20210218201041.65fknr7bdplwq...@linutronix.de/ > 3. bootargs isolcpus= > to isolate a cpu for a specific realtime task or interrupt > https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux_for_real_time/7/html/tuning_guide/isolating_cpus_using_tuned-profiles-realtime > 4. ARM FIQ which has separate fiq API, an example in fsl sound: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/fsl/imx-pcm-fiq.c > 5. Let one core invisible to Linux > Running non-os system and rtos on the core > Regarding Linux systems, it appears that approach 3 could theoretically achieve minimal interrupt latency for a given device without requiring any interrupt nesting. But the price is one CPU core which is not going to work on a uniprocessor system. > Honestly, I've never seen anyone who depends on irq priority to support > realtime in ARM Linux though ARM's RTOS-es use it quite commonly. > Perhaps you don't work with uniprocessor ARM Linux systems? > Once preempt_rt is enabled, those who want a fast irq environment need a > no_thread flag, or need to set its irq thread to higher sched_fifo/rr > priority. > Thanks for the tip. > [...] > > Anyway, the debate is long enough, let's move to some more important > things. I appreciate that you shared a lot of knowledge of m68k. > No problem. > Thanks > Barry >
RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers
On Tue, 23 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > Regarding m68k, your analysis overlooks the timing issue. E.g. patch > > 11/32 could be a problem because removing the irqsave would allow PDMA > > transfers to be interrupted. Aside from the timing issues, I agree > > with your analysis above regarding m68k. > > You mentioned you need realtime so you want an interrupt to be able to > preempt another one. That's not what I said. But for the sake of discussion, yes, I do know people who run Linux on ARM hardware (if Android vendor kernels can be called "Linux") and who would benefit from realtime support on those devices. > Now you said you want an interrupt not to be preempted as it will make a > timing issue. mac_esp deliberately constrains segment sizes so that it can harmlessly disable interrupts for the duration of the transfer. Maybe the irqsave in this driver is over-cautious. Who knows? The PDMA timing problem relates to SCSI bus signalling and the tolerance of real- world SCSI devices to same. The other problem is that the PDMA logic circuit is undocumented hardware. So there may be further timing requirements lurking there. Therefore, patch 11/32 is too risky. > If this PDMA transfer will have some problem when it is preempted, I > believe we need some enhanced ways to handle this, otherwise, once we > enable preempt_rt or threaded_irq, it will get the timing issue. so here > it needs a clear comment and IRQF_NO_THREAD if this is the case. > People who require fast response times cannot expect random drivers or platforms to meet such requirements. I fear you may be asking too much from Mac Quadra machines. > > > > With regard to other architectures and platforms, in specific cases, > > e.g. where there's never more than one IRQ involved, then I could > > agree that your assumptions probably hold and an irqsave would be > > probably redundant. > > > > When you find a redundant irqsave, to actually patch it would bring a > > risk of regression with little or no reward. It's not my place to veto > > this entire patch series on that basis but IMO this kind of churn is > > misguided. > > Nope. > > I would say the real misguidance is that the code adds one lock while it > doesn't need the lock. Easily we can add redundant locks or exaggerate > the coverage range of locks, but the smarter way is that people add > locks only when they really need the lock by considering concurrency and > realtime performance. > You appear to be debating a strawman. No-one is advocating excessive locking in new code. > Thanks > Barry >
RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers
On Mon, 22 Feb 2021, Song Bao Hua (Barry Song) wrote: > > On Thu, 18 Feb 2021, Xiaofei Tan wrote: > > > > > On 2021/2/9 13:06, Finn Thain wrote: > > > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > > > > > > > > > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI > > > > > > > drivers. There are no function changes, but may speed up if > > > > > > > interrupt happen too often. > > > > > > > > > > > > This change doesn't necessarily work on platforms that support > > > > > > nested interrupts. > > > > > > > > > > > > Were you able to measure any benefit from this change on some > > > > > > other platform? > > > > > > > > > > I think the code disabling irq in hardIRQ is simply wrong. Since > > > > > this commit > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc > > > > > genirq: Run irq handlers with interrupts disabled > > > > > > > > > > interrupt handlers are definitely running in a irq-disabled > > > > > context unless irq handlers enable them explicitly in the > > > > > handler to permit other interrupts. > > > > > > > > > > > > > Repeating the same claim does not somehow make it true. If you put > > > > your claim to the test, you'll see that that interrupts are not > > > > disabled on m68k when interrupt handlers execute. > > > > > > > > The Interrupt Priority Level (IPL) can prevent any given irq > > > > handler from being re-entered, but an irq with a higher priority > > > > level may be handled during execution of a lower priority irq > > > > handler. > > > > > > > > sonic_interrupt() uses an irq lock within an interrupt handler to > > > > avoid issues relating to this. This kind of locking may be needed > > > > in the drivers you are trying to patch. Or it might not. > > > > Apparently, no-one has looked. > > > > > > > > > > According to your discussion with Barry, it seems that m68k is a > > > little different from other architecture, and this kind of > > > modification of this patch cannot be applied to m68k. So, could help > > > to point out which driver belong to m68k architecture in this patch > > > set of SCSI? I can remove them. > > > > > > > If you would claim that "there are no function changes" in your > > patches (as above) then the onus is on you to support that claim. > > > > I assume that there are some platforms on which your assumptions hold. > > > > With regard to drivers for those platforms, you might want to explain > > why your patches should be applied there, given that the existing code > > is superior for being more portable. > > I don't think it has nothing to do with portability. In the case of > sonic_interrupt() you pointed out, on m68k, there is a high-priority > interrupt can preempt low-priority interrupt, they will result in access > the same critical data. M68K's spin_lock_irqsave() can disable the > high-priority interrupt and avoid the race condition of the data. So the > case should not be touched. I'd like to accept the reality and leave > sonic_interrupt() alone. > > However, even on m68k, spin_lock_irqsave is not needed for other > ordinary cases. > If there is no other irq handler coming to access same critical data, > it is pointless to hold a redundant irqsave lock in irqhandler even > on m68k. > > In thread contexts, we always need that if an irqhandler can preempt > those threads and access the same data. In hardirq, if there is an > high-priority which can jump out on m68k to access the critical data > which needs protection, we use the spin_lock_irqsave as you have used in > sonic_interrupt(). Otherwise, the irqsave is also redundant for m68k. > > > > > > BTW, sonic_interrupt() is from net driver natsemi, right? It would > > > be appreciative if only discuss SCSI drivers in this patch set. > > > thanks. > > > > > > > The 'net' subsystem does have some different requirements than the > > 'scsi' subsystem. But I don't see how that's relevant. Perhaps you can > > explain it. Thanks. > > The difference is that if there are two co-existing in
Re: [RFC] IRQ handlers run with some high-priority interrupts(not NMI) enabled on some platform
On Thu, 18 Feb 2021, Arnd Bergmann wrote: > On Thu, Feb 18, 2021 at 6:30 AM Finn Thain wrote: > > On Wed, 17 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > On Sat, 13 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > That scenario seems a little contrived to me (drivers for two or > > > > more devices sharing state through their interrupt handlers). Is > > > > it real? I suppose every platform has its quirks. The irq lock in > > > > sonic_interrupt() is only there because of a platform quirk (the > > > > same device can trigger either of two IRQs). Anyway, no-one > > > > expects all drivers to work on all platforms; I don't know why it > > > > bothers you so much when platforms differ. > > > > > > Basically, we wrote drivers with the assumption that this driver > > > will be cross-platform. (Of course there are some drivers which can > > > only work on one platform, for example, if the IP of the device is > > > only used in one platform as an internal component of a specific > > > SoC.) > > > > > > So once a device has two or more interrupts, we need to consider one > > > interrupt might preempt another one on m68k on the same cpu if we > > > also want to support this driver on m68k. this usually doesn't > > > matter on other platforms. > > > > When users show up who desire to run your drivers on their platform, > > you can expect them to bring patches and a MAINTAINERS file entry. > > AFAIK, Linux development has always worked that way. > > This is only part of the picture though. We also also constantly trying > to generalize the internal interfaces, to make sure that platforms work > the same way and that it's possible to write drivers in a portable way > without having to rely on platform maintainers to point out the > differences. > > I think it would make a lot of sense to remove the architecture > differences here by making m68k work the same way as the others and > documenting that as the expected behavior. > If you had some great new feature that was incompatible with priority masking, or incompatible with existing drivers portable enough to support such features, then I would be more amenable to your plan to remove functionality. But there's no real justification here. You say platform maintainers should not have to "point out the differences". But is that not their job? > You are probably right that there are no specific bugs on m68k machines > that rely on the nested hardirqs today, but I think they only get away > with it because > > a) there is no SMP support on m68k, so it likely doesn't run into the >more subtle cases with lock ordering that you could get when you have >hardirq handlers on multiple CPUs in parallel > And that's relevant because SMP support is now mandatory? Is this the logical consequence of your intention to "remove the architecture differences"? > b) there is a very limited number of device drivers that are actually >used on m68k, in particular only M54xx has PCI support, but that in >turn has a different interrupt scheme. > Everyone is afraid of some mysterious bug somewhere, yet no one can point to it. Again, I submit that the bug doesn't exist. That's because there is no material difference in semantics between the irqs_disabled() implementation that says "all interrupts are disabled except for NMI (and some others that some ARM platform cares about)" and the implementation that says "interrupts are disabled except higher priority ones than you may be enabled". If you can point to code that cares about such semantics, I predict you've found either a coding anti-pattern or perhaps some obscure hardware design flaw. Either way, there is no justification for your plan. > Changing the behavior on m68k clearly has its own regression risk, but > it could be done as a configuration option that defaults to the > traditional behavior on machines that have not been verified to be > well-behaved without nested hardirqs, and hidden on machines that do not > need it (any more). > This plan will quantifiably increase interrupt latency. It's not some vague risk that you can hand-wave away. It's unavoidable. > As far as I can tell, the only reason you would actually need nested > hardirqs is when a low-priority interrupt has to perform expensive I/O > processing, but we've had countless other methods to do the same over > the years (at least bottom half, softirq, taskqueue, tasklet, keventd, > workqueue, kthread, threaded interrupt handlers and probably others). > Nope. Interrupt priority masking is there to
Re: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers
On Thu, 18 Feb 2021, Xiaofei Tan wrote: > On 2021/2/9 13:06, Finn Thain wrote: > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > > > > > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI > > > > > drivers. There are no function changes, but may speed up if > > > > > interrupt happen too often. > > > > > > > > This change doesn't necessarily work on platforms that support > > > > nested interrupts. > > > > > > > > Were you able to measure any benefit from this change on some > > > > other platform? > > > > > > I think the code disabling irq in hardIRQ is simply wrong. > > > Since this commit > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc > > > genirq: Run irq handlers with interrupts disabled > > > > > > interrupt handlers are definitely running in a irq-disabled context > > > unless irq handlers enable them explicitly in the handler to permit > > > other interrupts. > > > > > > > Repeating the same claim does not somehow make it true. If you put > > your claim to the test, you'll see that that interrupts are not > > disabled on m68k when interrupt handlers execute. > > > > The Interrupt Priority Level (IPL) can prevent any given irq handler > > from being re-entered, but an irq with a higher priority level may be > > handled during execution of a lower priority irq handler. > > > > sonic_interrupt() uses an irq lock within an interrupt handler to > > avoid issues relating to this. This kind of locking may be needed in > > the drivers you are trying to patch. Or it might not. Apparently, > > no-one has looked. > > > > According to your discussion with Barry, it seems that m68k is a little > different from other architecture, and this kind of modification of this > patch cannot be applied to m68k. So, could help to point out which > driver belong to m68k architecture in this patch set of SCSI? I can > remove them. > If you would claim that "there are no function changes" in your patches (as above) then the onus is on you to support that claim. I assume that there are some platforms on which your assumptions hold. With regard to drivers for those platforms, you might want to explain why your patches should be applied there, given that the existing code is superior for being more portable. > BTW, sonic_interrupt() is from net driver natsemi, right? It would be > appreciative if only discuss SCSI drivers in this patch set. thanks. > The 'net' subsystem does have some different requirements than the 'scsi' subsystem. But I don't see how that's relevant. Perhaps you can explain it. Thanks.
RE: [RFC] IRQ handlers run with some high-priority interrupts(not NMI) enabled on some platform
On Wed, 17 Feb 2021, Song Bao Hua (Barry Song) wrote:
> > On Sat, 13 Feb 2021, Song Bao Hua (Barry Song) wrote:
> >
> > >
> > > So what is really confusing and a pain to me is that: For years
> > > people like me have been writing device drivers with the idea that
> > > irq handlers run with interrupts disabled after those commits in
> > > genirq. So I don't need to care about if some other IRQs on the same
> > > cpu will jump out to access the data the current IRQ handler is
> > > accessing.
> > >
> > > but it turns out the assumption is not true on some platform. So
> > > should I start to program devices driver with the new idea
> > > interrupts can actually come while irqhandler is running?
> > >
> > > That's the question which really bothers me.
> > >
> >
> > That scenario seems a little contrived to me (drivers for two or more
> > devices sharing state through their interrupt handlers). Is it real? I
> > suppose every platform has its quirks. The irq lock in
> > sonic_interrupt() is only there because of a platform quirk (the same
> > device can trigger either of two IRQs). Anyway, no-one expects all
> > drivers to work on all platforms; I don't know why it bothers you so
> > much when platforms differ.
>
> Basically, we wrote drivers with the assumption that this driver will be
> cross-platform. (Of course there are some drivers which can only work on
> one platform, for example, if the IP of the device is only used in one
> platform as an internal component of a specific SoC.)
>
> So once a device has two or more interrupts, we need to consider one
> interrupt might preempt another one on m68k on the same cpu if we also
> want to support this driver on m68k. this usually doesn't matter on
> other platforms.
>
When users show up who desire to run your drivers on their platform, you
can expect them to bring patches and a MAINTAINERS file entry. AFAIK,
Linux development has always worked that way.
Besides, not all m68k platforms implement priority masking. So there's no
problem with portability to m68k per se.
> on the other hand, there are more than 400 irqs_disabled() in kernel, I
> am really not sure if they are running with the knowledge that the true
> irqs_disabled() actually means some interrupts are off and some others
> are still open on m68k.
Firstly, use of irqs_disabled() is considered an antipattern by some
developers. Please see,
https://lore.kernel.org/linux-scsi/X8pfD5XtLoOygdez@lx-t490/
and
commit e6b6be53ec91 ("Merge branch 'net-in_interrupt-cleanup-and-fixes'")
This means that the differences in semantics between the irqs_disabled()
implementations on various architectures are moot.
Secondly, the existence of irqs_disabled() call sites does not imply a
flaw in your drivers nor in the m68k interrupt scheme. The actual semantic
differences are immaterial at many (all?) of these call sites.
> Or they are running with the assumption that the true irqs_disabled()
> means IRQ is totally quiet? If the latter is true, those drivers might
> fail to work on m68k as well.
>
Yes it's possible, and that was my fear too back in 2017 when I raised the
same question with the m68k maintainer. But I never found any code making
that assumption. If you know of such a bug, do tell. So far, your fears
remain unfounded.
> Thanks
> Barry
>
Re: [RFC] IRQ handlers run with some high-priority interrupts(not NMI) enabled on some platform
On Mon, 15 Feb 2021, Andy Shevchenko wrote: > On Sun, Feb 14, 2021 at 7:12 AM Finn Thain wrote: > > On Sat, 13 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > So what is really confusing and a pain to me is that: > > > For years people like me have been writing device drivers > > > with the idea that irq handlers run with interrupts > > > disabled after those commits in genirq. So I don't need > > > to care about if some other IRQs on the same cpu will > > > jump out to access the data the current IRQ handler > > > is accessing. > > > > > > but it turns out the assumption is not true on some platform. > > > So should I start to program devices driver with the new idea > > > interrupts can actually come while irqhandler is running? > > > > > > That's the question which really bothers me. > > > > > > > That scenario seems a little contrived to me (drivers for two or more > > devices sharing state through their interrupt handlers). Is it real? > > I suppose every platform has its quirks. The irq lock in sonic_interrupt() > > is only there because of a platform quirk (the same device can trigger > > either of two IRQs). Anyway, no-one expects all drivers to work on all > > platforms; I don't know why it bothers you so much when platforms differ. > > Isn't it any IRQ chip driver which may get IRQ on one or more lines > simultaneously? > The question here is can the handler be re-entrant on the same CPU in > that case? > An irq_chip handler used only for interrupts having the same priority level cannot be re-entered, thanks to the priority mask. Where the priority levels are different, as in auto_irq_chip in arch/m68k/kernel/ints.c, handle_simple_irq() can be re-entered but not with the same descriptor (i.e. no shared state). Documentation/core-api/genericirq.rst says, The locking of chip registers is up to the architecture that defines the chip primitives. The per-irq structure is protected via desc->lock, by the generic layer. Since the synchronization of chip registers is left up to the architecture, I think the related code should be portable. Looking in kernel/irq/*.c, I see that desc->lock is sometimes acquired with raw_spin_lock_irq(>lock) and sometimes raw_spin_lock(>lock). I don't know whether there are portability issues lurking here; this code is subtle and largely unfamiliar to me.
RE: [RFC] IRQ handlers run with some high-priority interrupts(not NMI) enabled on some platform
On Sat, 13 Feb 2021, Song Bao Hua (Barry Song) wrote: > > So what is really confusing and a pain to me is that: > For years people like me have been writing device drivers > with the idea that irq handlers run with interrupts > disabled after those commits in genirq. So I don't need > to care about if some other IRQs on the same cpu will > jump out to access the data the current IRQ handler > is accessing. > > but it turns out the assumption is not true on some platform. > So should I start to program devices driver with the new idea > interrupts can actually come while irqhandler is running? > > That's the question which really bothers me. > That scenario seems a little contrived to me (drivers for two or more devices sharing state through their interrupt handlers). Is it real? I suppose every platform has its quirks. The irq lock in sonic_interrupt() is only there because of a platform quirk (the same device can trigger either of two IRQs). Anyway, no-one expects all drivers to work on all platforms; I don't know why it bothers you so much when platforms differ.
RE: Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers
On Fri, 12 Feb 2021, Song Bao Hua (Barry Song) wrote:
>
> > -Original Message-
> > From: Finn Thain [mailto:fth...@telegraphics.com.au]
> > Sent: Friday, February 12, 2021 12:57 PM
> > To: Song Bao Hua (Barry Song)
> > Cc: tanxiaofei ; j...@linux.ibm.com;
> > martin.peter...@oracle.com; linux-s...@vger.kernel.org;
> > linux-kernel@vger.kernel.org; linux...@openeuler.org;
> > linux-m...@vger.kernel.org
> > Subject: RE: Re: [PATCH for-next 00/32] spin lock usage optimization for
> > SCSI
> > drivers
> >
> >
> > On Thu, 11 Feb 2021, Song Bao Hua (Barry Song) wrote:
> >
> > >
> > > Actually in m68k, I also saw its IRQ entry disabled interrupts by
> > > ' move#0x2700,%sr /* disable intrs */'
> > >
> > > arch/m68k/include/asm/entry.h:
> > >
> > > .macro SAVE_ALL_SYS
> > > move#0x2700,%sr /* disable intrs */
> > > btst#5,%sp@(2) /* from user? */
> > > bnes6f /* no, skip */
> > > movel %sp,sw_usp /* save user sp */
> > > ...
> > >
> > > .macro SAVE_ALL_INT
> > > SAVE_ALL_SYS
> > > moveq #-1,%d0 /* not system call entry */
> > > movel %d0,%sp@(PT_OFF_ORIG_D0)
> > > .endm
> > >
> > > arch/m68k/kernel/entry.S:
> > >
> > > /* This is the main interrupt handler for autovector interrupts */
> > >
> > > ENTRY(auto_inthandler)
> > > SAVE_ALL_INT
> > > GET_CURRENT(%d0)
> > > | put exception # in d0
> > > bfextu %sp@(PT_OFF_FORMATVEC){#4,#10},%d0
> > > subw#VEC_SPUR,%d0
> > >
> > > movel %sp,%sp@-
> > > movel %d0,%sp@- | put vector # on stack
> > > auto_irqhandler_fixup = . + 2
> > > jsr do_IRQ | process the IRQ
> > > addql #8,%sp | pop parameters off stack
> > > jra ret_from_exception
> > >
> > > So my question is that " move #0x2700,%sr" is actually disabling
> > > all interrupts? And is m68k actually running irq handlers
> > > with interrupts disabled?
> > >
> >
> > When sonic_interrupt() executes, the IPL is 2 or 3 (since either IRQ may
> > be involved). That is, SR & 0x700 is 0x200 or 0x300. The level 3 interrupt
> > may interrupt execution of the level 2 handler so an irq lock is used to
> > avoid re-entrance.
> >
> > This patch,
> >
> > diff --git a/drivers/net/ethernet/natsemi/sonic.c
> > b/drivers/net/ethernet/natsemi/sonic.c
> > index d17d1b4f2585..041354647bad 100644
> > --- a/drivers/net/ethernet/natsemi/sonic.c
> > +++ b/drivers/net/ethernet/natsemi/sonic.c
> > @@ -355,6 +355,8 @@ static irqreturn_t sonic_interrupt(int irq, void
> > *dev_id)
> > */
> > spin_lock_irqsave(>lock, flags);
> >
> > + printk_once(KERN_INFO "%s: %08lx\n", __func__, flags);
> > +
> > status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT;
> > if (!status) {
> > spin_unlock_irqrestore(>lock, flags);
> >
> > produces this output,
> >
> > [3.80] sonic_interrupt: 2300
>
> I actually hope you can directly read the register rather than reading
> a flag which might be a software one not from register.
>
Again, the implementation of arch_local_irq_save() may be found in
arch/m68k/include/asm/irqflags.h
> >
> > I ran that code in QEMU, but experience shows that Apple hardware works
> > exactly the same. Please do confirm this for yourself, if you still think
> > the code and comments in sonic_interrupt are wrong.
> >
> > > Best Regards
> > > Barry
> > >
>
> Thanks
> Barry
>
>
RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers
On Thu, 11 Feb 2021, Song Bao Hua (Barry Song) wrote: > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > > TBH, that is why m68k is so confusing. irqs_disabled() on m68k > > > > > should just reflect the status of all interrupts have been > > > > > disabled except NMI. > > > > > > > > > > irqs_disabled() should be consistent with the calling of APIs > > > > > such as local_irq_disable, local_irq_save, spin_lock_irqsave > > > > > etc. > > > > > > > > > > > > > When irqs_disabled() returns true, we cannot infer that > > > > arch_local_irq_disable() was called. But I have not yet found > > > > driver code or core kernel code attempting that inference. > > > > > > > > > > > > > > > > > Isn't arch_irqs_disabled() a status reflection of irq > > > > > > > disable API? > > > > > > > > > > > > > > > > > > > Why not? > > > > > > > > > > If so, arch_irqs_disabled() should mean all interrupts have been > > > > > masked except NMI as NMI is unmaskable. > > > > > > > > > > > > > Can you support that claim with a reference to core kernel code or > > > > documentation? (If some arch code agrees with you, that's neither > > > > here nor there.) > > > > > > I think those links I share you have supported this. Just you don't > > > believe :-) > > > > > > > Your links show that the distinction between fast and slow handlers > > was removed. Your links don't support your claim that > > "arch_irqs_disabled() should mean all interrupts have been masked". > > Where is the code that makes that inference? Where is the > > documentation that supports your claim? > > (1) > https://lwn.net/Articles/380931/ > Looking at all these worries, one might well wonder if a system which > *disabled interrupts for all handlers* would function well at all. So it > is interesting to note one thing: any system which has the lockdep > locking checker enabled has been running all handlers that way for some > years now. Many developers and testers run lockdep-enabled kernels, and > they are available for some of the more adventurous distributions > (Rawhide, for example) as well. So we have quite a bit of test coverage > for this mode of operation already. > IIUC, your claim is that CONFIG_LOCKDEP involves code that contains the inference, "arch_irqs_disabled() means all interrupts have been masked". Unfortunately, m68k lacks CONFIG_LOCKDEP support so I can't easily confirm this. I suppose there may be other architectures that support both LOCKDEP and nested interrupts (?) Anyway, if you would point to the code that contains said inference, that would help a lot. > (2) > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b738a50a > > "We run all handlers *with interrupts disabled* and expect them not to > enable them. Warn when we catch one who does." > Again, you don't see that warning because irqs_disabled() correctly returns true. You can confirm this fact in QEMU or Aranym if you are interested. > (3) > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc > genirq: Run irq handlers *with interrupts disabled* > > Running interrupt handlers with interrupts enabled can cause stack > overflows. That has been observed with multiqueue NICs delivering all > their interrupts to a single core. We might band aid that somehow by > checking the interrupt stacks, but the real safe fix is to *run the irq > handlers with interrupts disabled*. > Again, the stack overflow issue is not applicable. 68000 uses a priority mask, like ARM GIC. So there's no arbitrary nesting of interrupt handlers. In practice stack overflows simply don't occur on m68k. Please do try it. > > All these documents say we are running irq handler with interrupts > disabled. but it seems you think high-prio interrupts don't belong > to "interrupts" in those documents :-) > > that is why we can't get agreement. I think "interrupts" mean all except > NMI in these documents, but you insist high-prio IRQ is an exception. > We can't get agreement because you seek to remove functionality without justification. > > > > > > > > > > > > > > > > Are all interrupts (including NMI) masked whenever > > > > > > arch_irqs_disabled() returns true on your platforms? > > > > > > > > > > On my platform, once irqs_disabled() is true, all interrupts are > > > > > masked except NMI. NMI just ignore spin_lock_irqsave or > > > > > local_irq_disable. > > > > > > > > > > On ARM64, we also have high-priority interrupts, but they are > > > > > running as PESUDO_NMI: > > > > > https://lwn.net/Articles/755906/ > > > > > > > > > > > > > A glance at the ARM GIC specification suggests that your hardware > > > > works much like 68000 hardware. > > > > > > > >When enabled, a CPU interface takes the highest priority > > > >pending interrupt for its connected processor and determines > > > >whether
RE: Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers
On Thu, 11 Feb 2021, Song Bao Hua (Barry Song) wrote:
>
> Actually in m68k, I also saw its IRQ entry disabled interrupts by
> ' move#0x2700,%sr /* disable intrs */'
>
> arch/m68k/include/asm/entry.h:
>
> .macro SAVE_ALL_SYS
> move#0x2700,%sr /* disable intrs */
> btst#5,%sp@(2) /* from user? */
> bnes6f /* no, skip */
> movel %sp,sw_usp /* save user sp */
> ...
>
> .macro SAVE_ALL_INT
> SAVE_ALL_SYS
> moveq #-1,%d0 /* not system call entry */
> movel %d0,%sp@(PT_OFF_ORIG_D0)
> .endm
>
> arch/m68k/kernel/entry.S:
>
> /* This is the main interrupt handler for autovector interrupts */
>
> ENTRY(auto_inthandler)
> SAVE_ALL_INT
> GET_CURRENT(%d0)
> | put exception # in d0
> bfextu %sp@(PT_OFF_FORMATVEC){#4,#10},%d0
> subw#VEC_SPUR,%d0
>
> movel %sp,%sp@-
> movel %d0,%sp@- | put vector # on stack
> auto_irqhandler_fixup = . + 2
> jsr do_IRQ | process the IRQ
> addql #8,%sp | pop parameters off stack
> jra ret_from_exception
>
> So my question is that " move #0x2700,%sr" is actually disabling
> all interrupts? And is m68k actually running irq handlers
> with interrupts disabled?
>
When sonic_interrupt() executes, the IPL is 2 or 3 (since either IRQ may
be involved). That is, SR & 0x700 is 0x200 or 0x300. The level 3 interrupt
may interrupt execution of the level 2 handler so an irq lock is used to
avoid re-entrance.
This patch,
diff --git a/drivers/net/ethernet/natsemi/sonic.c
b/drivers/net/ethernet/natsemi/sonic.c
index d17d1b4f2585..041354647bad 100644
--- a/drivers/net/ethernet/natsemi/sonic.c
+++ b/drivers/net/ethernet/natsemi/sonic.c
@@ -355,6 +355,8 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
*/
spin_lock_irqsave(>lock, flags);
+ printk_once(KERN_INFO "%s: %08lx\n", __func__, flags);
+
status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT;
if (!status) {
spin_unlock_irqrestore(>lock, flags);
produces this output,
[3.80] sonic_interrupt: 2300
I ran that code in QEMU, but experience shows that Apple hardware works
exactly the same. Please do confirm this for yourself, if you still think
the code and comments in sonic_interrupt are wrong.
> Best Regards
> Barry
>
RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers
On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > > > There is no warning from m68k builds. That's because > > > > > > arch_irqs_disabled() returns true when the IPL is non-zero. > > > > > > > > > > So for m68k, the case is arch_irqs_disabled() is true, but > > > > > interrupts can still come? > > > > > > > > > > Then it seems it is very confusing. If prioritized interrupts > > > > > can still come while arch_irqs_disabled() is true, > > > > > > > > Yes, on m68k CPUs, an IRQ having a priority level higher than the > > > > present priority mask will get serviced. > > > > > > > > Non-Maskable Interrupt (NMI) is not subject to this rule and gets > > > > serviced regardless. > > > > > > > > > how could spin_lock_irqsave() block the prioritized interrupts? > > > > > > > > It raises the the mask level to 7. Again, please see > > > > arch/m68k/include/asm/irqflags.h > > > > > > Hi Finn, > > > Thanks for your explanation again. > > > > > > TBH, that is why m68k is so confusing. irqs_disabled() on m68k > > > should just reflect the status of all interrupts have been disabled > > > except NMI. > > > > > > irqs_disabled() should be consistent with the calling of APIs such > > > as local_irq_disable, local_irq_save, spin_lock_irqsave etc. > > > > > > > When irqs_disabled() returns true, we cannot infer that > > arch_local_irq_disable() was called. But I have not yet found driver > > code or core kernel code attempting that inference. > > > > > > > > > > > Isn't arch_irqs_disabled() a status reflection of irq disable > > > > > API? > > > > > > > > > > > > > Why not? > > > > > > If so, arch_irqs_disabled() should mean all interrupts have been > > > masked except NMI as NMI is unmaskable. > > > > > > > Can you support that claim with a reference to core kernel code or > > documentation? (If some arch code agrees with you, that's neither here > > nor there.) > > I think those links I share you have supported this. Just you don't > believe :-) > Your links show that the distinction between fast and slow handlers was removed. Your links don't support your claim that "arch_irqs_disabled() should mean all interrupts have been masked". Where is the code that makes that inference? Where is the documentation that supports your claim? > > > > > > > > > > Are all interrupts (including NMI) masked whenever > > > > arch_irqs_disabled() returns true on your platforms? > > > > > > On my platform, once irqs_disabled() is true, all interrupts are > > > masked except NMI. NMI just ignore spin_lock_irqsave or > > > local_irq_disable. > > > > > > On ARM64, we also have high-priority interrupts, but they are > > > running as PESUDO_NMI: > > > https://lwn.net/Articles/755906/ > > > > > > > A glance at the ARM GIC specification suggests that your hardware > > works much like 68000 hardware. > > > >When enabled, a CPU interface takes the highest priority pending > >interrupt for its connected processor and determines whether the > >interrupt has sufficient priority for it to signal the interrupt > >request to the processor. [...] > > > >When the processor acknowledges the interrupt at the CPU interface, > >the Distributor changes the status of the interrupt from pending to > >either active, or active and pending. At this point the CPU > >interface can signal another interrupt to the processor, to preempt > >interrupts that are active on the processor. If there is no pending > >interrupt with sufficient priority for signaling to the processor, > >the interface deasserts the interrupt request signal to the > >processor. > > > > https://developer.arm.com/documentation/ihi0048/b/ > > > > Have you considered that Linux/arm might benefit if it could fully > > exploit hardware features already available, such as the interrupt > > priority masking feature in the GIC in existing arm systems? > > I guess no:-) there are only two levels: IRQ and NMI. Injecting a > high-prio IRQ level between them makes no sense. > > To me, arm64's design is quite clear and has no any confusion. > Are you saying that the ARM64 hardware design is confusing because it implements a priority mask, and that's why you had to simplify it with a pseudo-nmi scheme in software? > > > > > On m68k, it seems you mean： > > > irq_disabled() is true, but high-priority interrupts can still come; > > > local_irq_disable() can disable high-priority interrupts, and at that > > > time, irq_disabled() is also true. > > > > > > TBH, this is wrong and confusing on m68k. > > > > > > > Like you, I was surprised when I learned about it. But that doesn't mean > > it's wrong. The fact that it works should tell you something. > > > > The fact is that m68k lets arch_irq_disabled() return true to pretend > all IRQs are disabled while high-priority IRQ
RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers
On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > > > There is no warning from m68k builds. That's because > > > > arch_irqs_disabled() returns true when the IPL is non-zero. > > > > > > So for m68k, the case is > > > arch_irqs_disabled() is true, but interrupts can still come? > > > > > > Then it seems it is very confusing. If prioritized interrupts can > > > still come while arch_irqs_disabled() is true, > > > > Yes, on m68k CPUs, an IRQ having a priority level higher than the > > present priority mask will get serviced. > > > > Non-Maskable Interrupt (NMI) is not subject to this rule and gets > > serviced regardless. > > > > > how could spin_lock_irqsave() block the prioritized interrupts? > > > > It raises the the mask level to 7. Again, please see > > arch/m68k/include/asm/irqflags.h > > Hi Finn, > Thanks for your explanation again. > > TBH, that is why m68k is so confusing. irqs_disabled() on m68k should > just reflect the status of all interrupts have been disabled except NMI. > > irqs_disabled() should be consistent with the calling of APIs such as > local_irq_disable, local_irq_save, spin_lock_irqsave etc. > When irqs_disabled() returns true, we cannot infer that arch_local_irq_disable() was called. But I have not yet found driver code or core kernel code attempting that inference. > > > > > Isn't arch_irqs_disabled() a status reflection of irq disable API? > > > > > > > Why not? > > If so, arch_irqs_disabled() should mean all interrupts have been masked > except NMI as NMI is unmaskable. > Can you support that claim with a reference to core kernel code or documentation? (If some arch code agrees with you, that's neither here nor there.) > > > > Are all interrupts (including NMI) masked whenever > > arch_irqs_disabled() returns true on your platforms? > > On my platform, once irqs_disabled() is true, all interrupts are masked > except NMI. NMI just ignore spin_lock_irqsave or local_irq_disable. > > On ARM64, we also have high-priority interrupts, but they are running as > PESUDO_NMI: > https://lwn.net/Articles/755906/ > A glance at the ARM GIC specification suggests that your hardware works much like 68000 hardware. When enabled, a CPU interface takes the highest priority pending interrupt for its connected processor and determines whether the interrupt has sufficient priority for it to signal the interrupt request to the processor. [...] When the processor acknowledges the interrupt at the CPU interface, the Distributor changes the status of the interrupt from pending to either active, or active and pending. At this point the CPU interface can signal another interrupt to the processor, to preempt interrupts that are active on the processor. If there is no pending interrupt with sufficient priority for signaling to the processor, the interface deasserts the interrupt request signal to the processor. https://developer.arm.com/documentation/ihi0048/b/ Have you considered that Linux/arm might benefit if it could fully exploit hardware features already available, such as the interrupt priority masking feature in the GIC in existing arm systems? > On m68k, it seems you mean： > irq_disabled() is true, but high-priority interrupts can still come; > local_irq_disable() can disable high-priority interrupts, and at that > time, irq_disabled() is also true. > > TBH, this is wrong and confusing on m68k. > Like you, I was surprised when I learned about it. But that doesn't mean it's wrong. The fact that it works should tell you something. Things could always be made simpler. But discarding features isn't necessarily an improvement. > > > > > Thanks > > > Barry > > > > > Thanks > Barry >
RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers
On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote:
> > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote:
> >
> > > > > sonic_interrupt() uses an irq lock within an interrupt handler
> > > > > to avoid issues relating to this. This kind of locking may be
> > > > > needed in the drivers you are trying to patch. Or it might not.
> > > > > Apparently, no-one has looked.
> > >
> > > Is the comment in sonic_interrupt() outdated according to this:
> > > m68k: irq: Remove IRQF_DISABLED
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=77a4279
> > >
> > > http://lkml.iu.edu/hypermail/linux/kernel/1109.2/01687.html
> > >
> >
> > The removal of IRQF_DISABLED isn't relevant to this driver. Commit
> > 77a42796786c ("m68k: Remove deprecated IRQF_DISABLED") did not disable
> > interrupts, it just removed some code to enable them.
> >
> > The code and comments in sonic_interrupt() are correct. You can
> > confirm this for yourself quite easily using QEMU and a
> > cross-compiler.
> >
> > > and this: genirq: Warn when handler enables interrupts
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b738a50a
> > >
> > > wouldn't genirq report a warning on m68k?
> > >
> >
> > There is no warning from m68k builds. That's because
> > arch_irqs_disabled() returns true when the IPL is non-zero.
>
>
> So for m68k, the case is
> arch_irqs_disabled() is true, but interrupts can still come?
>
> Then it seems it is very confusing. If prioritized interrupts can still
> come while arch_irqs_disabled() is true,
Yes, on m68k CPUs, an IRQ having a priority level higher than the present
priority mask will get serviced.
Non-Maskable Interrupt (NMI) is not subject to this rule and gets serviced
regardless.
> how could spin_lock_irqsave() block the prioritized interrupts?
It raises the the mask level to 7. Again, please see
arch/m68k/include/asm/irqflags.h
> Isn't arch_irqs_disabled() a status reflection of irq disable API?
>
Why not?
Are all interrupts (including NMI) masked whenever arch_irqs_disabled()
returns true on your platforms?
> Thanks
> Barry
>
>
RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers
On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote:
> > > sonic_interrupt() uses an irq lock within an interrupt handler to
> > > avoid issues relating to this. This kind of locking may be needed in
> > > the drivers you are trying to patch. Or it might not. Apparently,
> > > no-one has looked.
>
> Is the comment in sonic_interrupt() outdated according to this:
> m68k: irq: Remove IRQF_DISABLED
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=77a4279
> http://lkml.iu.edu/hypermail/linux/kernel/1109.2/01687.html
>
The removal of IRQF_DISABLED isn't relevant to this driver. Commit
77a42796786c ("m68k: Remove deprecated IRQF_DISABLED") did not disable
interrupts, it just removed some code to enable them.
The code and comments in sonic_interrupt() are correct. You can confirm
this for yourself quite easily using QEMU and a cross-compiler.
> and this:
> genirq: Warn when handler enables interrupts
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b738a50a
>
> wouldn't genirq report a warning on m68k?
>
There is no warning from m68k builds. That's because arch_irqs_disabled()
returns true when the IPL is non-zero.
> >
> > Thanks
> > Barry
>
> Thanks
> Barry
>
>
RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers
On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > > > > > > > > > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI > > > > > > > drivers. There are no function changes, but may speed up if > > > > > > > interrupt happen too often. > > > > > > > > > > > > This change doesn't necessarily work on platforms that support > > > > > > nested interrupts. > > > > > > > > > > > > Were you able to measure any benefit from this change on some > > > > > > other platform? > > > > > > > > > > I think the code disabling irq in hardIRQ is simply wrong. Since > > > > > this commit > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc > > > > > > > > > > genirq: Run irq handlers with interrupts disabled > > > > > > > > > > interrupt handlers are definitely running in a irq-disabled > > > > > context unless irq handlers enable them explicitly in the > > > > > handler to permit other interrupts. > > > > > > > > > > > > > Repeating the same claim does not somehow make it true. > > > > > > Sorry for I didn't realize xiaofei had replied. > > > > > > > I was referring to the claim in patch 00/32, i.e. that interrupt > > handlers only run when irqs are disabled. > > > > > > If you put your claim to the test, you'll see that that interrupts > > > > are not disabled on m68k when interrupt handlers execute. > > > > > > Sounds like an implementation issue of m68k since IRQF_DISABLED has > > > been totally removed. > > > > > > > It's true that IRQF_DISABLED could be used to avoid the need for irq > > locks in interrupt handlers. So, if you want to remove irq locks from > > interrupt handlers, today you can't use IRQF_DISABLED to help you. So > > what? > > > > > > > > > > The Interrupt Priority Level (IPL) can prevent any given irq > > > > handler from being re-entered, but an irq with a higher priority > > > > level may be handled during execution of a lower priority irq > > > > handler. > > > > > > > > > > We used to have IRQF_DISABLED to support so-called "fast interrupt" > > > to avoid this. > > > > > > But the concept has been totally removed. That is interesting if > > > m68k still has this issue. > > > > > > > Prioritized interrupts are beneficial. Why would you want to avoid > > them? > > > > I doubt this is true as it has been already thought as unnecessary > in Linux: > https://lwn.net/Articles/380931/ > The article you cited does not refute what I said about prioritized interrupts. The article is about eliminating the distinction between fast and slow interrupt handlers. The article says that some developers convinced Linus that, although minimal interrupt latency is desirable, is isn't strictly necessary. The article also warns of stack overflow from arbitrarily deep slow interrupt nesting, but that's not what m68k does. > > Moreover, there's no reason to believe that m68k is the only platform > > that supports nested interrupts. > > I doubt that is true as genirq is running understand the consumption > that hardIRQ is running in irq-disabled context: I'm not going to guess whether other platforms might be affected -- you're supporting this patch so you will have to show that it is correct. > "We run all handlers with interrupts disabled and expect them not to > enable them. Warn when we catch one who does." > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b738a50a > > If it is, m68k is against the assumption of genirq. > Interrupt handlers on m68k do not enable interrupts. If they did, you would see that warning fire. It doesn't fire. Try it. > > > > > > sonic_interrupt() uses an irq lock within an interrupt handler to > > > > avoid issues relating to this. This kind of locking may be needed in > > > > the drivers you are trying to patch. Or it might not. Apparently, > > > > no-one has looked. > > > > > Thanks > Barry >
RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers
On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > > > > > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI > > > > > drivers. There are no function changes, but may speed up if > > > > > interrupt happen too often. > > > > > > > > This change doesn't necessarily work on platforms that support > > > > nested interrupts. > > > > > > > > Were you able to measure any benefit from this change on some > > > > other platform? > > > > > > I think the code disabling irq in hardIRQ is simply wrong. Since > > > this commit > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc > > > > > > genirq: Run irq handlers with interrupts disabled > > > > > > interrupt handlers are definitely running in a irq-disabled context > > > unless irq handlers enable them explicitly in the handler to permit > > > other interrupts. > > > > > > > Repeating the same claim does not somehow make it true. > > Sorry for I didn't realize xiaofei had replied. > I was referring to the claim in patch 00/32, i.e. that interrupt handlers only run when irqs are disabled. > > If you put your claim to the test, you'll see that that interrupts are > > not disabled on m68k when interrupt handlers execute. > > Sounds like an implementation issue of m68k since IRQF_DISABLED has been > totally removed. > It's true that IRQF_DISABLED could be used to avoid the need for irq locks in interrupt handlers. So, if you want to remove irq locks from interrupt handlers, today you can't use IRQF_DISABLED to help you. So what? > > > > The Interrupt Priority Level (IPL) can prevent any given irq handler > > from being re-entered, but an irq with a higher priority level may be > > handled during execution of a lower priority irq handler. > > > > We used to have IRQF_DISABLED to support so-called "fast interrupt" to > avoid this. > > But the concept has been totally removed. That is interesting if m68k > still has this issue. > Prioritized interrupts are beneficial. Why would you want to avoid them? Moreover, there's no reason to believe that m68k is the only platform that supports nested interrupts. > > sonic_interrupt() uses an irq lock within an interrupt handler to > > avoid issues relating to this. This kind of locking may be needed in > > the drivers you are trying to patch. Or it might not. Apparently, > > no-one has looked. > > Thanks > Barry >
Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers
On Tue, 9 Feb 2021, tanxiaofei wrote: > Hi Finn, > Thanks for reviewing the patch set. > > On 2021/2/8 15:57, Finn Thain wrote: > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI drivers. > > > There are no function changes, but may speed up if interrupt happen too > > > often. > > > > This change doesn't necessarily work on platforms that support nested > > interrupts. > > > > Linux doesn't support nested interrupts anymore after the following > patch, so please don't worry this. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc > Clearly that patch did not disable interrupts. It removed a statement that enabled them. > > Were you able to measure any benefit from this change on some other > > platform? > > > > It's hard to measure the benefit of this change. It's hard to see any benefit. But it's easy to see risk, when there's no indication that you've confirmed that the affected drivers do not rely on the irq lock, nor tested them for regressions, nor checked whether the affected platforms meet your assumuptions. > Hmm, you could take this patch set as cleanup. thanks. > A "cleanup" does not change program behaviour. Can you demonstrate that program behaviour is unchanged? > > Please see also, > > https://lore.kernel.org/linux-scsi/89c5cb05cb844939ae684db0077f6...@h3c.com/ > > > > . > > > >
RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers
On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > -Original Message- > > From: Finn Thain [mailto:fth...@telegraphics.com.au] > > Sent: Monday, February 8, 2021 8:57 PM > > To: tanxiaofei > > Cc: j...@linux.ibm.com; martin.peter...@oracle.com; > > linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org; > > linux...@openeuler.org > > Subject: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization > > for SCSI drivers > > > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI drivers. > > > There are no function changes, but may speed up if interrupt happen too > > > often. > > > > This change doesn't necessarily work on platforms that support nested > > interrupts. > > > > Were you able to measure any benefit from this change on some other > > platform? > > I think the code disabling irq in hardIRQ is simply wrong. > Since this commit > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc > genirq: Run irq handlers with interrupts disabled > > interrupt handlers are definitely running in a irq-disabled context > unless irq handlers enable them explicitly in the handler to permit > other interrupts. > Repeating the same claim does not somehow make it true. If you put your claim to the test, you'll see that that interrupts are not disabled on m68k when interrupt handlers execute. The Interrupt Priority Level (IPL) can prevent any given irq handler from being re-entered, but an irq with a higher priority level may be handled during execution of a lower priority irq handler. sonic_interrupt() uses an irq lock within an interrupt handler to avoid issues relating to this. This kind of locking may be needed in the drivers you are trying to patch. Or it might not. Apparently, no-one has looked.
Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers
On Sun, 7 Feb 2021, Xiaofei Tan wrote: > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI drivers. > There are no function changes, but may speed up if interrupt happen too > often. This change doesn't necessarily work on platforms that support nested interrupts. Were you able to measure any benefit from this change on some other platform? Please see also, https://lore.kernel.org/linux-scsi/89c5cb05cb844939ae684db0077f6...@h3c.com/
[PATCH] m68k: Drop -fno-strength-reduce from KBUILD_CFLAGS
This workaround became redundant either when the driver in question was
removed (in Linux v2.6.23) or when the compiler flag became a no-op
(in GCC v4.2). Linux has required GCC v4.6 or later since v4.19.
Link:
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=efa1cdf01850b28c2f6f7035ebd4420259494615
References: commit 565bae6a4a8f ("[SCSI] 53c7xx: kill driver")
References: commit cafa0010cd51 ("Raise the minimum required gcc version to
4.6")
Signed-off-by: Finn Thain
---
arch/m68k/Makefile | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/m68k/Makefile b/arch/m68k/Makefile
index ea14f2046fb4..5be4efec173a 100644
--- a/arch/m68k/Makefile
+++ b/arch/m68k/Makefile
@@ -66,8 +66,7 @@ KBUILD_CFLAGS += $(cpuflags-y)
KBUILD_CFLAGS += -pipe -ffreestanding
ifdef CONFIG_MMU
-# without -fno-strength-reduce the 53c7xx.c driver fails ;-(
-KBUILD_CFLAGS += -fno-strength-reduce -ffixed-a2
+KBUILD_CFLAGS += -ffixed-a2
else
# we can use a m68k-linux-gcc toolchain with these in place
KBUILD_CPPFLAGS += -DUTS_SYSNAME=\"uClinux\"
--
2.26.2
Re: Old platforms never die, was Re: Old platforms: bring out your dead
On Sat, 16 Jan 2021, Rob Landley wrote: > Speaking of which, my qemu m68k system has failed to boot ever since commit: > > commit f93bfeb55255bddaa16597e187a99ae6131b964a > Author: Finn Thain > Date: Sun Jun 28 14:23:12 2020 +1000 > > macintosh/via-macii: Poll the device most likely to respond > Yes, that patch series broke your emulator by fixing kernel bugs. Please refer to this message for some background and some solutions-- https://lore.kernel.org/linux-m68k/alpine.LNX.2.23.453.2008100844450.8@nippy.intranet/
Undesirable code, was Re: Old platforms etc.
On Thu, 14 Jan 2021, Arnd Bergmann wrote: > I think it's mainly a misunderstanding of what I am trying to do > in finding the platforms that have been completely abandoned. Have you tried to identify those drivers and Kconfig symbols in mainline that are used only by devices that don't function with mainline kernels?
New platforms: bring out your dead, was Re: Old platforms: bring out your dead
On Wed, 13 Jan 2021, Arnd Bergmann wrote: > > It's usually one of two things that happened before a platform gets > deleted for good: > > * The platform port was (almost) exclusively done by one company >with a commercial interest in it, and the company shifts its priorities >for some reason (acquisition, bankruptcy, product cancellation, >accidentally laying off all competent developers, ...) that causes it to >stop working on it. Sometimes the previously paid maintainers >keep up their upstream position, but without someone pushing the >last missing bits into an official release, users are stuck on an old >BSP kernel. > Yes, that's the typical product life cycle. It presumes a short window of opportunity for sales (suggesting that demand is not organic). Most vendors like to avoid having to compete with their own prior product lines. Hence the constrained lifespan that we get from devices with out-of-tree Linux ports. AFAICS open source licenses cannot really prevent this (no matter how many freedoms the FSF would like to confer on people). However, consumer law could do it, if it were to disallow products which were not servicable. > * A platform port is done in the open and actually works for upstream > users, but over time the last active maintainers move on in their > lives. Complex platforms inevitably break when a treewide change > goes wrong, so we rely on users that are able to bisect and report > bugs when they happen. And without bug reports, breakage gets leveraged into deletion, using the bogus argument that "broken" code is proof of zero potential users. > After a platform has been broken for too long, even competent users > may decide to just give up and stay on their last working kernel. Some > of these platforms eventually recover when a new maintainer steps up > or someone discovers they depend on newer kernels for products, but > after a few years it's usually beyond repair. > So all a vendor has to do is make maintenance a bit too difficult for competent users e.g. by depriving them of access to existing documentation. It was only a few decades ago that every applicance you bought came with a printed circuit schematic. These days, every device you buy comes with built-in obsolescence and a customer lock-in strategy. > Arnd >
Old platforms never die, was Re: Old platforms: bring out your dead
On Tue, 12 Jan 2021, John Paul Adrian Glaubitz wrote: > > There has to be a healthy balance between hobbyist and commercial use. > Yes, both of those, and everything in-between, including for-profit businesses that serve mostly hobbyists. Also start-up companies that may never be commercially viable (which is most of them). And don't forget government and non-government organisations, not-for-profit organisations, charities, etc. > I understand that from a commercial point of view, it doesn't make much > sense to run Linux on a 30-year-old computer. It ain't necessarily so. I would be surprised if there are no Linux VMs running on old corporate mainframes right now. But the age of the hardware is largely irrelevant. If you're a museum interested in cultural artifacts from decades past, or if you're a business doing data recovery, you're going to need to operate those platforms. Once removed from mainline Linux, a port becomes basically frozen, and may not be compatible with future emulators, which are a moving target. I say that because last year I fixed bugs in Linux/m68k that made it incomatible with recent QEMU releases (it was only compatible with old QEMU releases).
Re: [PATCH] scsi: target/sbp: remove firewire SBP target driver
On Mon, 4 Jan 2021, Bart Van Assche wrote: > On 6/16/20 7:07 PM, Finn Thain wrote: > > On Tue, 16 Jun 2020, Bart Van Assche wrote: > >> As far as I know the sbp driver only has had one user ever and that > >> user is no longer user the sbp driver. > > > > So, you estimate the userbase at zero. Can you give a confidence > > level? Actual measurement is hard because when end users encounter > > breakage, they look for quick workarounds before they undertake post > > mortem, log collection, bug reporting, mailing list discussions, > > analysis etc. > > (replying to an e-mail from six months ago) > > Hi Finn, > > I am confident that my estimate is an accurate estimate since I have not > seen any sbp support requests, sbp bug reports nor any sbp bug fixes > since the sbp target driver has been accepted upstream. > That suggests to me that the code that you're hoping to remove 1) has no bugs, or 2) has no reported bugs, or 3) has no users at present. I am confident that your evidence does not support your conclusion (i.e. the code will never be used again). Sometimes, users only appear after the unreported bugs get fixed. I've seen it happen. > > Here's a different question: "Why remove it from the kernel tree?" > > > > If maintaining this code is a burden, is it not the kind of tax that > > all developers/users pay to all developers/users? Does this driver > > impose an unreasonably high burden for some reason? > > Yes. If anyone wants to change the interface between SCSI target core > and SCSI target drivers, all target drivers, including the sbp and FCoE > target driver have to be retested. I'm unaware of such an obligation. API changes happen often. When they do, we see good test coverage of commercially viable hardware, some best-effort testing of common hardware, and some perfunctory build testing. But that is missing the point, which was about a particular driver, not about development process. You have not shown how the target API is special, to support your claim that this driver imposes an unreasonable burden. In the interests of making forward progress in this discussion, shall we discuss the kind of SCSI Target API changes that you anticipate? > In other words, keeping unused target drivers inside the kernel tree > involves a significant maintenance burden for anyone who wants to modify > the interface between the SCSI target core and SCSI target drivers. > Keeping _any_ driver in the kernel involves a maintenance burden. There are two good ways to address that. Firstly, by improving the development process. For example, an API change is mostly mechanical work that lends itself to automated refactoring. Secondly, by involving all interested parties, so that the burden is shared. Of course, there are other ways. E.g. "don't ship code when doing so won't turn a profit". That, by the way, was the policy that gave us 10 billion Android devices (or more) that don't function with a mainline kernel. > Additionally, there is a good alternative available for the sbp driver. > Every system I know of that is equipped with a Firewire port also has an > Ethernet port. So users who want to provide SCSI target functionality on > such systems can use any SCSI transport protocol that is compatible with > Ethernet (iSCSI, iSER over soft-RoCE, SRP over soft-RoCE, ...). > Ethernet is not always an alternative. That was already discussed in this thread. But let's assume for a moment that you can migrate any and all users of this driver over to an ethernet driver. Why would the maintainers of that ethernet driver and its API accept that plan, if adding users would extend their maintenance and testing obligations? Do you think those maintainers should pay the "kind of tax that all developers/users pay to all developers/users?" > Thanks, > > Bart. >
[PATCH v4 net RESEND] net/sonic: Fix some resource leaks in error handling paths
From: Christophe JAILLET
A call to dma_alloc_coherent() is wrapped by sonic_alloc_descriptors().
This is correctly freed in the remove function, but not in the error
handling path of the probe function. Fix this by adding the missing
dma_free_coherent() call.
While at it, rename a label in order to be slightly more informative.
Cc: Christophe JAILLET
Cc: Thomas Bogendoerfer
Cc: Chris Zankel
References: commit 10e3cc180e64 ("net/sonic: Fix a resource leak in an error
handling path in 'jazz_sonic_probe()'")
Fixes: 74f2a5f0ef64 ("xtensa: Add support for the Sonic Ethernet device for the
XT2000 board.")
Fixes: efcce839360f ("[PATCH] macsonic/jazzsonic network drivers update")
Signed-off-by: Christophe JAILLET
Signed-off-by: Finn Thain
---
Changed since v3:
- Update commit log.
Changed since v2:
- Dropped whitespace change.
---
drivers/net/ethernet/natsemi/macsonic.c | 12 ++--
drivers/net/ethernet/natsemi/xtsonic.c | 7 +--
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/natsemi/macsonic.c
b/drivers/net/ethernet/natsemi/macsonic.c
index 776b7d264dc3..2289e1fe3741 100644
--- a/drivers/net/ethernet/natsemi/macsonic.c
+++ b/drivers/net/ethernet/natsemi/macsonic.c
@@ -506,10 +506,14 @@ static int mac_sonic_platform_probe(struct
platform_device *pdev)
err = register_netdev(dev);
if (err)
- goto out;
+ goto undo_probe;
return 0;
+undo_probe:
+ dma_free_coherent(lp->device,
+ SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
+ lp->descriptors, lp->descriptors_laddr);
out:
free_netdev(dev);
@@ -584,12 +588,16 @@ static int mac_sonic_nubus_probe(struct nubus_board
*board)
err = register_netdev(ndev);
if (err)
- goto out;
+ goto undo_probe;
nubus_set_drvdata(board, ndev);
return 0;
+undo_probe:
+ dma_free_coherent(lp->device,
+ SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
+ lp->descriptors, lp->descriptors_laddr);
out:
free_netdev(ndev);
return err;
diff --git a/drivers/net/ethernet/natsemi/xtsonic.c
b/drivers/net/ethernet/natsemi/xtsonic.c
index afa166ff7aef..28d9e98db81a 100644
--- a/drivers/net/ethernet/natsemi/xtsonic.c
+++ b/drivers/net/ethernet/natsemi/xtsonic.c
@@ -229,11 +229,14 @@ int xtsonic_probe(struct platform_device *pdev)
sonic_msg_init(dev);
if ((err = register_netdev(dev)))
- goto out1;
+ goto undo_probe1;
return 0;
-out1:
+undo_probe1:
+ dma_free_coherent(lp->device,
+ SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
+ lp->descriptors, lp->descriptors_laddr);
release_region(dev->base_addr, SONIC_MEM_SIZE);
out:
free_netdev(dev);
--
2.26.2
Re: [PATCH RESEND] ide/falconide: Fix module unload
Hi All,
This patch was sent in September and subsequently resent in November. I've
since learned that the maintainer has been ill. What's the best way
forward for fixes like this?
Regards,
Finn
On Fri, 20 Nov 2020, Finn Thain wrote:
> Unloading the falconide module results in a crash:
>
> Unable to handle kernel NULL pointer dereference at virtual address
> Oops:
> Modules linked in: falconide(-)
> PC: [<002930b2>] ide_host_remove+0x2e/0x1d2
> SR: 2000 SP: 00b49e28 a2: 009b0f90
> d0: d1: 009b0f90d2: d3: 00b48000
> d4: 003cef32d5: 00299188a0: 0086d000a1: 0086d000
> Process rmmod (pid: 322, task=009b0f90)
> Frame format=7 eff addr= ssw=0505 faddr=
> wb 1 stat/addr/data:
> wb 2 stat/addr/data:
> wb 3 stat/addr/data: 00018da9
> push data:
> Stack from 00b49e90:
> 004c456a 0027f176 0027cb0a 0027cb9e 0086d00a 2187d3f0
> 0027f0e0
> 00b49ebc 2187d1f6 00b49ec8 002811e8 0086d000 00b49ef0
> 0028024c
> 0086d00a 002800d6 00279a1a 0001 0001 0086d00a 2187d3f0
> 00279a58
> 00b49f1c 002802e0 0086d00a 2187d3f0 004c456a 0086d00a ef96af74
>
> 2187d3f0 002805d2 800de064 00b49f44 0027f088 2187d3f0 00ac1cf4
> 2187d3f0
> 004c43be 2187d3f0 2187d3f0 800b66a8 00b49f5c 00280776
> 2187d3f0
> Call Trace: [<0027f176>] __device_driver_unlock+0x0/0x48
> [<0027cb0a>] device_links_busy+0x0/0x94
> [<0027cb9e>] device_links_unbind_consumers+0x0/0x130
> [<0027f0e0>] __device_driver_lock+0x0/0x5a
> [<2187d1f6>] falconide_remove+0x12/0x18 [falconide]
> [<002811e8>] platform_drv_remove+0x1c/0x28
> [<0028024c>] device_release_driver_internal+0x176/0x17c
> [<002800d6>] device_release_driver_internal+0x0/0x17c
> [<00279a1a>] get_device+0x0/0x22
> [<00279a58>] put_device+0x0/0x18
> [<002802e0>] driver_detach+0x56/0x82
> [<002805d2>] driver_remove_file+0x0/0x24
> [<0027f088>] bus_remove_driver+0x4c/0xa4
> [<00280776>] driver_unregister+0x28/0x5a
> [<00281a00>] platform_driver_unregister+0x12/0x18
> [<2187d2a0>] ide_falcon_driver_exit+0x10/0x16 [falconide]
> [<000764f0>] sys_delete_module+0x110/0x1f2
> [<000e83ea>] sys_rename+0x1a/0x1e
> [<2e0c>] syscall+0x8/0xc
> [<00188004>] ext4_multi_mount_protect+0x35a/0x3ce
> Code: 0029 9188 4bf9 0027 aa1c 283c 003c ef32 <265c> 4a8b 6700 00b8 2043 2028
> 000c 0280 00ff ff00 6600 0176 40c0 7202 b2b9 004c
> Disabling lock debugging due to kernel taint
>
> This happens because the driver_data pointer is uninitialized.
> Add the missing platform_set_drvdata() call. For clarity, use the
> matching platform_get_drvdata() as well.
>
> Cc: Michael Schmitz
> Cc: Bartlomiej Zolnierkiewicz
> Fixes: 5ed0794cde593 ("m68k/atari: Convert Falcon IDE drivers to platform
> drivers")
> Reviewed-by: Geert Uytterhoeven
> Reviewed-by: Michael Schmitz
> Signed-off-by: Finn Thain
> ---
> This patch was tested using Aranym.
> ---
> drivers/ide/falconide.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/ide/falconide.c b/drivers/ide/falconide.c
> index dbeb2605e5f6..607c44bc50f1 100644
> --- a/drivers/ide/falconide.c
> +++ b/drivers/ide/falconide.c
> @@ -166,6 +166,7 @@ static int __init falconide_init(struct platform_device
> *pdev)
> if (rc)
> goto err_free;
>
> + platform_set_drvdata(pdev, host);
> return 0;
> err_free:
> ide_host_free(host);
> @@ -176,7 +177,7 @@ static int __init falconide_init(struct platform_device
> *pdev)
>
> static int falconide_remove(struct platform_device *pdev)
> {
> - struct ide_host *host = dev_get_drvdata(>dev);
> + struct ide_host *host = platform_get_drvdata(pdev);
>
> ide_host_remove(host);
>
>
Re: [PATCH v3] scsi: NCR5380: Remove context check
On Sun, 6 Dec 2020, Ahmed S. Darwish wrote: > NCR5380_poll_politely2() uses in_interrupt() and irqs_disabled() to > check if it is safe to sleep. > > Such usage in drivers is phased out and Linus clearly requested that > code which changes behaviour depending on context should either be > separated, or the context be explicitly conveyed in an argument passed > by the caller. > > Below is a context analysis of NCR5380_poll_politely2() uppermost > callers: > > - NCR5380_maybe_reset_bus(), task, invoked during device probe. > -> NCR5380_poll_politely() > -> do_abort() > > - NCR5380_select(), task, but can only sleep in the "release, then > re-acquire" regions of the spinlock held by its caller. > Sleeping invocations (lock released): > -> NCR5380_poll_politely2() > > Atomic invocations (lock acquired): > -> NCR5380_reselect() >-> NCR5380_poll_politely() >-> do_abort() >-> NCR5380_transfer_pio() > > - NCR5380_intr(), interrupt handler > -> NCR5380_dma_complete() >-> NCR5380_transfer_pio() > -> NCR5380_poll_politely() > -> NCR5380_reselect() (see above) > > - NCR5380_information_transfer(), task, but can only sleep in the > "release, then re-acquire" regions of the caller-held spinlock. > Sleeping invocations (lock released): > - NCR5380_transfer_pio() -> NCR5380_poll_politely() > - NCR5380_poll_politely() > > Atomic invocations (lock acquired): > - NCR5380_transfer_dma() > -> NCR5380_dma_recv_setup() >=> generic_NCR5380_precv() -> NCR5380_poll_politely() > => macscsi_pread() -> NCR5380_poll_politely() > > -> NCR5380_dma_send_setup() > => generic_NCR5380_psend -> NCR5380_poll_politely2() > => macscsi_pwrite() -> NCR5380_poll_politely() > > -> NCR5380_poll_politely2() > -> NCR5380_dma_complete() >-> NCR5380_transfer_pio() > -> NCR5380_poll_politely() > - NCR5380_transfer_pio() -> NCR5380_poll_politely > > - NCR5380_reselect(), atomic, always called with hostdata spinlock > held. > > Since NCR5380_poll_politely2() already takes a "wait" argument in > jiffies, use it to determine if the function can sleep. Modify atomic > callers, which passed an unused wait value in terms of HZ, to pass zero. > > Suggested-by: Finn Thain > Co-developed-by: Sebastian Andrzej Siewior > Signed-off-by: Ahmed S. Darwish > Cc: Michael Schmitz > Cc: Acked-by: Finn Thain
Re: [PATCH v2] scsi: NCR5380: Remove context check
On Fri, 4 Dec 2020, Ahmed S. Darwish wrote:
> NCR5380_poll_politely2() uses in_interrupt() and irqs_disabled() to
> check if it is safe to sleep.
>
> Such usage in drivers is phased out and Linus clearly requested that
> code which changes behaviour depending on context should either be
> separated, or the context be explicitly conveyed in an argument passed
> by the caller.
>
> Below is a context analysis of NCR5380_poll_politely2() uppermost
> callers:
>
> - NCR5380_maybe_reset_bus(), task, invoked during device probe.
> -> NCR5380_poll_politely()
> -> do_abort()
>
> - NCR5380_select(), task, but can only sleep in the "release, then
> re-acquire" regions of the spinlock held by its caller.
> Sleeping invocations (lock released):
> -> NCR5380_poll_politely2()
>
> Atomic invocations (lock acquired):
> -> NCR5380_reselect()
>-> NCR5380_poll_politely()
>-> do_abort()
>-> NCR5380_transfer_pio()
>
> - NCR5380_intr(), interrupt handler
> -> NCR5380_dma_complete()
>-> NCR5380_transfer_pio()
> -> NCR5380_poll_politely()
> -> NCR5380_reselect() (see above)
>
> - NCR5380_information_transfer(), task, but can only sleep in the
> "release, then re-acquire" regions of the caller-held spinlock.
> Sleeping invocations (lock released):
> - NCR5380_transfer_pio() -> NCR5380_poll_politely()
> - NCR5380_poll_politely()
>
> Atomic invocations (lock acquired):
> - NCR5380_transfer_dma()
> -> NCR5380_dma_recv_setup()
>=> generic_NCR5380_precv() -> NCR5380_poll_politely()
> => macscsi_pread() -> NCR5380_poll_politely()
>
> -> NCR5380_dma_send_setup()
> => generic_NCR5380_psend -> NCR5380_poll_politely2()
> => macscsi_pwrite() -> NCR5380_poll_politely()
>
> -> NCR5380_poll_politely2()
> -> NCR5380_dma_complete()
>-> NCR5380_transfer_pio()
> -> NCR5380_poll_politely()
> - NCR5380_transfer_pio() -> NCR5380_poll_politely
>
> - NCR5380_reselect(), atomic, always called with hostdata spinlock
> held.
>
> Since NCR5380_poll_politely2() already takes a "wait" argument in
> jiffies, use it to determine if the function can sleep. Modify atomic
> callers, which passed an unused wait value in terms of HZ, to pass zero.
>
> Suggested-by: Finn Thain
> Co-developed-by: Sebastian Andrzej Siewior
> Signed-off-by: Ahmed S. Darwish
> Cc: Michael Schmitz
> Cc:
> ---
> drivers/scsi/NCR5380.c | 77 ++--
> drivers/scsi/NCR5380.h | 3 +-
> drivers/scsi/g_NCR5380.c | 12 +++
> drivers/scsi/mac_scsi.c | 10 +++---
> 4 files changed, 55 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
> index d654a6cc4162..60200f61592e 100644
> --- a/drivers/scsi/NCR5380.c
> +++ b/drivers/scsi/NCR5380.c
> @@ -132,7 +132,7 @@
> static unsigned int disconnect_mask = ~0;
> module_param(disconnect_mask, int, 0444);
>
> -static int do_abort(struct Scsi_Host *);
> +static int do_abort(struct Scsi_Host *, unsigned int);
> static void do_reset(struct Scsi_Host *);
> static void bus_reset_cleanup(struct Scsi_Host *);
>
> @@ -197,7 +197,7 @@ static inline void set_resid_from_SCp(struct scsi_cmnd
> *cmd)
> * @reg2: Second 5380 register to poll
> * @bit2: Second bitmask to check
> * @val2: Second expected value
> - * @wait: Time-out in jiffies
> + * @wait: Time-out in jiffies, 0 if sleeping is not allowed
> *
> * Polls the chip in a reasonably efficient manner waiting for an
> * event to occur. After a short quick poll we begin to yield the CPU
> @@ -213,7 +213,7 @@ static int NCR5380_poll_politely2(struct NCR5380_hostdata
> *hostdata,
>unsigned long wait)
> {
> unsigned long n = hostdata->poll_loops;
> - unsigned long deadline = jiffies + wait;
> + unsigned long deadline;
>
> do {
> if ((NCR5380_read(reg1) & bit1) == val1)
> @@ -223,10 +223,11 @@ static int NCR5380_poll_politely2(struct
> NCR5380_hostdata *hostdata,
> cpu_relax();
> } while (n--);
>
> - if (irqs_disabled() || in_interrupt())
> + if (!wait)
> return -ETIMEDOUT;
>
> /* Repeatedly sleep for 1 ms until deadline */
> + deadline = jiffies + wait;
> while (time_is_after_jiffies(deadline)) {
> schedule_timeout_uninterruptible(1);
> if ((NCR5380_read(reg1) & bit1) == val1)
The deadline assignment shouldn't be moved. That's a behavioural change
and doesn't fit under the stated aim of this patch. Also, it isn't
actually a desirable change: the argument to this function is the overall
wait time, not just the sleep limit.
The rest looks ok.
[PATCH] MAINTAINERS: Update 68k Mac entry
Two files under drivers/macintosh are actually m68k-only. I think that patches for these files should be reviewed in the appropriate forum and merged via the appropriate tree, rather than falling to the powerpc maintainers to deal with. Update the "M68K ON APPLE MACINTOSH" section accordingly. Cc: Michael Ellerman Cc: Benjamin Herrenschmidt Cc: Joshua Thompson Cc: linuxppc-...@lists.ozlabs.org Cc: linux-m...@lists.linux-m68k.org Signed-off-by: Finn Thain --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 867157311dc8..e8fa0c9645d6 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -10322,6 +10322,8 @@ L: linux-m...@lists.linux-m68k.org S: Maintained W: http://www.mac.linux-m68k.org/ F: arch/m68k/mac/ +F: drivers/macintosh/adb-iop.c +F: drivers/macintosh/via-macii.c M68K ON HP9000/300 M: Philip Blundell -- 2.26.2
Re: [PATCH] macintosh/adb-iop: Send correct poll command
On Fri, 4 Dec 2020, Geert Uytterhoeven wrote:
> Hi Finn,
>
> On Fri, Nov 20, 2020 at 5:54 AM Finn Thain wrote:
> > The behaviour of the IOP firmware is not well documented but we do know
> > that IOP message reply data can be used to issue new ADB commands.
> > Use the message reply to better control autopoll behaviour by sending
> > a Talk Register 0 command after every ADB response, not unlike the
> > algorithm in the via-macii driver. This poll command is addressed to
> > that device which last received a Talk command (explicit or otherwise).
> >
> > Cc: Joshua Thompson
> > Fixes: fa3b5a9929fc ("macintosh/adb-iop: Implement idle -> sending state
> > transition")
>
> WARNING: Unknown commit id 'fa3b5a9929fc', maybe rebased or not pulled?
>
> 32226e817043?
>
Yes, that's the one. I accidentally gave a commit id from one of my
backport branches.
> > Tested-by: Stan Johnson
> > Signed-off-by: Finn Thain
>
> Thanks, will queue in the m68k for-v5.11 branch.
>
Thanks.
> Gr{oetje,eeting}s,
>
> Geert
>
>
Re: [PATCH] macintosh/adb-iop: Always wait for reply message from IOP
On Fri, 4 Dec 2020, Geert Uytterhoeven wrote:
> Hi Finn,
>
> On Fri, Nov 20, 2020 at 5:54 AM Finn Thain wrote:
> > A recent patch incorrectly altered the adb-iop state machine behaviour
> > and introduced a regression that can appear intermittently as a
> > malfunctioning ADB input device. This seems to be caused when reply
> > packets from different ADB commands become mixed up, especially during
> > the adb bus scan. Fix this by unconditionally entering the awaiting_reply
> > state after sending an explicit command, even when the ADB command won't
> > generate a reply from the ADB device.
> >
> > Cc: Joshua Thompson
> > Fixes: e2954e5f727f ("macintosh/adb-iop: Implement sending -> idle state
> > transition")
> > Tested-by: Stan Johnson
> > Signed-off-by: Finn Thain
>
> Thanks for your patch!
>
> > --- a/drivers/macintosh/adb-iop.c
> > +++ b/drivers/macintosh/adb-iop.c
> > @@ -84,10 +84,7 @@ static void adb_iop_complete(struct iop_msg *msg)
> >
> > local_irq_save(flags);
> >
> > - if (current_req->reply_expected)
> > - adb_iop_state = awaiting_reply;
> > - else
> > - adb_iop_done();
> > + adb_iop_state = awaiting_reply;
> >
> > local_irq_restore(flags);
> > }
> > @@ -95,8 +92,9 @@ static void adb_iop_complete(struct iop_msg *msg)
> > /*
> > * Listen for ADB messages from the IOP.
> > *
> > - * This will be called when unsolicited messages (usually replies to TALK
> > - * commands or autopoll packets) are received.
> > + * This will be called when unsolicited IOP messages are received.
> > + * These IOP messages can carry ADB autopoll responses and also occur
> > + * after explicit ADB commands.
> > */
> >
> > static void adb_iop_listen(struct iop_msg *msg)
> > @@ -123,8 +121,10 @@ static void adb_iop_listen(struct iop_msg *msg)
> > if (adb_iop_state == awaiting_reply) {
> > struct adb_request *req = current_req;
> >
> > - req->reply_len = amsg->count + 1;
> > - memcpy(req->reply, >cmd, req->reply_len);
> > + if (req->reply_expected) {
> > + req->reply_len = amsg->count + 1;
> > + memcpy(req->reply, >cmd,
> > req->reply_len);
> > + }
>
> So if we're not expecting a reply. It's ignored.
> Just wondering: what kind of messages are being dropped?
I believe they were empty, with flags == ADB_IOP_EXPLICIT|ADB_IOP_TIMEOUT.
> If reply packets from different ADB commands become mixed up, they are
> still (expected?) replies to messages we sent before. Why shouldn't we
> depend on receiving the replies?
>
It turns out that the IOP always generates reply messages, even when the
ADB command does not produce a reply packet (e.g. ADB Listen command).
The commit being fixed got that wrong.
So it's not really the ADB reply packets that are being mixed up, it's the
IOP messages that enclose them. The bug goes like this:
1. CPU sends a message to the IOP, expecting no response because this
message contains an ADB Listen command. The ADB command is now considered
complete.
2. CPU sends a second message to the IOP, this time expecting a response
because this message contains an ADB Talk command. This ADB command needs
a reply before it can be completed.
3. adb-iop driver receives an IOP message and assumes that it relates to
the Talk command. It's actually for the previous command. The Talk command
is now considered complete but it gets the wrong reply data.
4. adb-iop driver gets another IOP response message, which contains the
actual reply data for the Talk command, but this is dropped (the driver is
no longer in awaiting_reply state).
Please go ahead and add this analysis to the commit log if you think it
would help.
> >
> > req_done = true;
> > }
>
> Gr{oetje,eeting}s,
>
> Geert
>
>
Re: [PATCH] scsi/NCR5380: Remove in_interrupt() test
136046n Tue, 1 Dec 2020, Sebastian Andrzej Siewior wrote: > On 2020-12-01 17:46:36 [+1100], Finn Thain wrote: > > The in_interrupt() macro is deprecated. Also, it's usage in > > NCR5380_poll_politely2() has long been redundant. > > So you rely on the assumption that interrupts are always disabled. Hmmm. > You complained about the additional argument and that things may get > wrong with it. > Now that I look at it, I realize that hostdata->poll_loops is used for > the initial poll and the `wait' argument is only used for sleeping. What > about it gets redefined to 0 if sleeping is not possible and != 0 if > sleeping is requested. > This results in a few callers passing 0 instead of HZ (or so) which > should make things more obvious. > > There is however do_abort(, HZ) and abort does internally "wait * 10" so > it matches the old value. > So the argument being passed is not simply "poll interval" in jiffies but "poll interval divided by 10"? That's a suprising API to chose (further comments below). > --->8-- > > From: "Ahmed S. Darwish" > Subject: [PATCH] scsi: NCR5380: Remove in_interrupt() usage. > > NCR5380_poll_politely2() uses in_interrupt() to check if it is safe to > sleep. > > The usage of in_interrupt() in drivers is phased out and Linus clearly > requested that code which changes behaviour depending on context should > either be separated, or the context be explicitly conveyed in an > argument passed by the caller. > > Below is a context analysis of NCR5380_poll_politely2() uppermost > callers: > > - NCR5380_maybe_reset_bus(), task, invoked during device probe. > -> NCR5380_poll_politely() > -> do_abort() > > - NCR5380_select(), task, but can only sleep in the "release, then > re-acquire" regions of the spinlock held by its caller. > Sleeping invocations (lock released): > -> NCR5380_poll_politely2() > > Atomic invocations (lock acquired): > -> NCR5380_reselect() >-> NCR5380_poll_politely() >-> do_abort() >-> NCR5380_transfer_pio() > > - NCR5380_intr(), interrupt handler > -> NCR5380_dma_complete() >-> NCR5380_transfer_pio() > -> NCR5380_poll_politely() > -> NCR5380_reselect() (see above) > > - NCR5380_information_transfer(), task, but can only sleep in the > "release, then re-acquire" regions of the caller-held spinlock. > Sleeping invocations (lock released): > - NCR5380_transfer_pio() -> NCR5380_poll_politely() > - NCR5380_poll_politely() > > Atomic invocations (lock acquired): > - NCR5380_transfer_dma() > -> NCR5380_dma_recv_setup() >=> generic_NCR5380_precv() -> NCR5380_poll_politely() > => macscsi_pread() -> NCR5380_poll_politely() > > -> NCR5380_dma_send_setup() > => generic_NCR5380_psend -> NCR5380_poll_politely2() > => macscsi_pwrite() -> NCR5380_poll_politely() > > -> NCR5380_poll_politely2() > -> NCR5380_dma_complete() >-> NCR5380_transfer_pio() > -> NCR5380_poll_politely() > - NCR5380_transfer_pio() -> NCR5380_poll_politely > > - NCR5380_reselect(), atomic, always called with hostdata spinlock > held. > > Use 0 in NCR5380_poll_politely2() delay argument if sleeping is not > possible. Otherwise pass the requested time out in jiffies. > > For the mixed ones, trickle-down context from upper layers. > > Signed-off-by: Ahmed S. Darwish > [bigeasy: remove the bool, make decision based on `wait' ] > Signed-off-by: Sebastian Andrzej Siewior > Cc: Finn Thain > Cc: Michael Schmitz > Cc: > --- > drivers/scsi/NCR5380.c | 74 ++-- > drivers/scsi/NCR5380.h | 4 ++- > drivers/scsi/g_NCR5380.c | 12 +++ > drivers/scsi/mac_scsi.c | 10 +++--- > 4 files changed, 54 insertions(+), 46 deletions(-) > > diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c > index d597d7493a627..7b0606d8f529a 100644 > --- a/drivers/scsi/NCR5380.c > +++ b/drivers/scsi/NCR5380.c > @@ -132,7 +132,7 @@ > static unsigned int disconnect_mask = ~0; > module_param(disconnect_mask, int, 0444); > > -static int do_abort(struct Scsi_Host *); > +static int do_abort(struct Scsi_Host *, unsigned long); > static void do_reset(struct Scsi_Host *); > static void bus_reset_cleanup(struct Scsi_Host *); > > @@ -197,7 +197,7 @@ static inline void set_resid_from_SCp(struct scsi_cmnd > *cmd) > * @reg2:
[PATCH] scsi/NCR5380: Remove in_interrupt() test
The in_interrupt() macro is deprecated. Also, it's usage in NCR5380_poll_politely2() has long been redundant. Cc: Sebastian Andrzej Siewior Cc: Ahmed S. Darwish Cc: Thomas Gleixner Link: https://lore.kernel.org/r/20201126132952.2287996-1-bige...@linutronix.de Signed-off-by: Finn Thain --- drivers/scsi/NCR5380.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c index 462d911a89f2..6972e7ceb81a 100644 --- a/drivers/scsi/NCR5380.c +++ b/drivers/scsi/NCR5380.c @@ -223,7 +223,10 @@ static int NCR5380_poll_politely2(struct NCR5380_hostdata *hostdata, cpu_relax(); } while (n--); - if (irqs_disabled() || in_interrupt()) + /* Sleeping is not allowed when in atomic or interrupt contexts. +* Callers in such contexts always disable local irqs. +*/ + if (irqs_disabled()) return -ETIMEDOUT; /* Repeatedly sleep for 1 ms until deadline */ -- 2.26.2
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, 25 Nov 2020, Nick Desaulniers wrote: > On Wed, Nov 25, 2020 at 1:33 PM Finn Thain wrote: > > > > Or do you think that a codebase can somehow satisfy multiple checkers > > and their divergent interpretations of the language spec? > > Have we found any cases yet that are divergent? I don't think so. You mean, aside from -Wimplicit-fallthrough? I'm glad you asked. How about -Wincompatible-pointer-types and -Wframe-larger-than? All of the following files have been affected by divergent diagnostics produced by clang and gcc. arch/arm64/include/asm/neon-intrinsics.h arch/powerpc/xmon/Makefile drivers/gpu/drm/i915/Makefile drivers/gpu/drm/i915/i915_utils.h drivers/staging/media/atomisp/pci/atomisp_subdev.c fs/ext4/super.c include/trace/events/qla.h net/mac80211/rate.c tools/lib/string.c tools/perf/util/setup.py tools/scripts/Makefile.include And if I searched for 'smatch' or 'coverity' instead of 'clang' I'd probably find more divergence. Here are some of the relevant commits. 0738c8b5915c7eaf1e6007b441008e8f3b460443 9c87156cce5a63735d1218f0096a65c50a7a32aa babaab2f473817f173a2d08e410c25abf5ed0f6b 065e5e559555e2f100bc95792a8ef1b609bbe130 93f56de259376d7e4fff2b2d104082e1fa66e237 6c4798d3f08b81c2c52936b10e0fa872590c96ae b7a313d84e853049062011d78cb04b6decd12f5c 093b75ef5995ea35d7f6bdb6c7b32a42a1999813 And before you object, "but -Wconstant-logical-operand is a clang-only warning! it can't be divergent with gcc!", consider that the special cases added to deal with clang-only warnings have to be removed when gcc catches up, which is more churn. Now multiply that by the number of checkers you care about.
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, 25 Nov 2020, Nick Desaulniers wrote: > On Wed, Nov 25, 2020 at 1:33 PM Finn Thain > wrote: > > > > Or do you think that a codebase can somehow satisfy multiple checkers > > and their divergent interpretations of the language spec? > > Have we found any cases yet that are divergent? I don't think so. There are many implementations, so I think you are guaranteed to find more divergence if you look. That's because the spec is full of language like this: "implementations are encouraged not to emit a diagnostic" and "implementations are encouraged to issue a diagnostic". Some implementations will decide to not emit (under the premise that vast amounts of existing code would have to get patched until the compiler goes quiet) whereas other implementations will decide to emit (under the premise that the author is doing the checking and not the janitor or the packager). > It sounds to me like GCC's cases it warns for is a subset of Clang's. > Having additional coverage with Clang then should ensure coverage for > both. > If that claim were true, the solution would be simple. (It's not.) For the benefit of projects that enable -Werror and projects that nominated gcc as their preferred compiler, clang would simply need a flag to enable conformance with gcc by suppressing those additional warnings that clang would normally produce. This simple solution is, of course, completely unworkable, since it would force clang to copy some portion of gcc's logic (rewritten under LLVM's unique license) and then to track future changes to that portion of gcc indefinitely.
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, 25 Nov 2020, Nick Desaulniers wrote: > So developers and distributions using Clang can't have > -Wimplicit-fallthrough enabled because GCC is less strict (which has > been shown in this thread to lead to bugs)? We'd like to have nice > things too, you know. > Apparently the GCC developers don't want you to have "nice things" either. Do you think that the kernel should drop gcc in favour of clang? Or do you think that a codebase can somehow satisfy multiple checkers and their divergent interpretations of the language spec? > This is not a shiny new warning; it's already on for GCC and has existed > in both compilers for multiple releases. > Perhaps you're referring to the compiler feature that lead to the ill-fated, tree-wide /* fallthrough */ patch series. When the ink dries on the C23 language spec and the implementations figure out how to interpret it then sure, enforce the warning for new code -- the cost/benefit analysis is straight forward. However, the case for patching existing mature code is another story.
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, 25 Nov 2020, Miguel Ojeda wrote: > > The C standard has nothing to do with this. We use compiler extensions > of several kinds, for many years. Even discounting those extensions, the > kernel is not even conforming to C due to e.g. strict aliasing. I am not > sure what you are trying to argue here. > I'm saying that supporting the official language spec makes more sense than attempting to support a multitude of divergent interpretations of the spec (i.e. gcc, clang, coverity etc.) I'm also saying that the reason why we use -std=gnu89 is that existing code was written in that language, not in ad hoc languages comprised of collections of extensions that change with every release. > But, since you insist: yes, the `fallthrough` attribute is in the > current C2x draft. > Thank you for checking. I found a free version that's only 6 weeks old: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2583.pdf It will be interesting to see whether 6.7.11.5 changes once the various implementations reach agreement.
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Tue, 24 Nov 2020, Kees Cook wrote: > On Mon, Nov 23, 2020 at 08:31:30AM -0800, James Bottomley wrote: > > Really, no ... something which produces no improvement has no value at > > all ... we really shouldn't be wasting maintainer time with it because > > it has a cost to merge. I'm not sure we understand where the balance > > lies in value vs cost to merge but I am confident in the zero value > > case. > > What? We can't measure how many future bugs aren't introduced because > the kernel requires explicit case flow-control statements for all new > code. > These statements are not "missing" unless you presume that code written before the latest de facto language spec was written should somehow be held to that spec. If the 'fallthrough' statement is not part of the latest draft spec then we should ask why not before we embrace it. Being that the kernel still prefers -std=gnu89 you might want to consider what has prevented -std=gnu99 or -std=gnu2x etc. > We already enable -Wimplicit-fallthrough globally, so that's not the > discussion. The issue is that Clang is (correctly) even more strict than > GCC for this, so these are the remaining ones to fix for full Clang > coverage too. > Seems to me you should be patching the compiler. When you have consensus among the language lawyers you'll have more credibility with those being subjected to enforcement.
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, 23 Nov 2020, Joe Perches wrote: > On Tue, 2020-11-24 at 11:58 +1100, Finn Thain wrote: > > it's not for me to prove that such patches don't affect code > > generation. That's for the patch author and (unfortunately) for > > reviewers. > > Ideally, that proof would be provided by the compilation system itself > and not patch authors nor reviewers nor maintainers. > > Unfortunately gcc does not guarantee repeatability or deterministic > output. To my knowledge, neither does clang. > Yes, I've said the same thing myself. But having attempted it, I now think this is a hard problem. YMMV. https://lore.kernel.org/linux-scsi/alpine.LNX.2.22.394.2004281017310.12@nippy.intranet/ https://lore.kernel.org/linux-scsi/alpine.LNX.2.22.394.2005211358460.8@nippy.intranet/
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, 23 Nov 2020, Miguel Ojeda wrote:
> On Mon, 23 Nov 2020, Finn Thain wrote:
>
> > On Sun, 22 Nov 2020, Miguel Ojeda wrote:
> >
> > >
> > > It isn't that much effort, isn't it? Plus we need to take into
> > > account the future mistakes that it might prevent, too.
> >
> > We should also take into account optimisim about future improvements
> > in tooling.
> >
> Not sure what you mean here. There is no reliable way to guess what the
> intention was with a missing fallthrough, even if you parsed whitespace
> and indentation.
>
What I meant was that you've used pessimism as if it was fact.
For example, "There is no way to guess what the effect would be if the
compiler trained programmers to add a knee-jerk 'break' statement to avoid
a warning".
Moreover, what I meant was that preventing programmer mistakes is a
problem to be solved by development tools. The idea that retro-fitting new
language constructs onto mature code is somehow necessary to "prevent
future mistakes" is entirely questionable.
> > > So even if there were zero problems found so far, it is still a
> > > positive change.
> > >
> >
> > It is if you want to spin it that way.
> >
> How is that a "spin"? It is a fact that we won't get *implicit*
> fallthrough mistakes anymore (in particular if we make it a hard error).
>
Perhaps "handwaving" is a better term?
> > > I would agree if these changes were high risk, though; but they are
> > > almost trivial.
> > >
> >
> > This is trivial:
> >
> > case 1:
> > this();
> > + fallthrough;
> > case 2:
> > that();
> >
> > But what we inevitably get is changes like this:
> >
> > case 3:
> > this();
> > + break;
> > case 4:
> > hmmm();
> >
> > Why? Mainly to silence the compiler. Also because the patch author
> > argued successfully that they had found a theoretical bug, often in
> > mature code.
> >
> If someone changes control flow, that is on them. Every kernel developer
> knows what `break` does.
>
Sure. And if you put -Wimplicit-fallthrough into the Makefile and if that
leads to well-intentioned patches that cause regressions, it is partly on
you.
Have you ever considered the overall cost of the countless
-Wpresume-incompetence flags?
Perhaps you pay the power bill for a build farm that produces logs that
no-one reads? Perhaps you've run git bisect, knowing that the compiler
messages are not interesting? Or compiled software in using a language
that generates impenetrable messages? If so, here's a tip:
# grep CFLAGS /etc/portage/make.conf
CFLAGS="... -Wno-all -Wno-extra ..."
CXXFLAGS="${CFLAGS}"
Now allow me some pessimism: the hardware upgrades, gigawatt hours and
wait time attributable to obligatory static analyses are a net loss.
> > But is anyone keeping score of the regressions? If unreported bugs
> > count, what about unreported regressions?
> >
> Introducing `fallthrough` does not change semantics. If you are really
> keen, you can always compare the objects because the generated code
> shouldn't change.
>
No, it's not for me to prove that such patches don't affect code
generation. That's for the patch author and (unfortunately) for reviewers.
> Cheers,
> Miguel
>
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, 22 Nov 2020, Miguel Ojeda wrote: > > It isn't that much effort, isn't it? Plus we need to take into account > the future mistakes that it might prevent, too. We should also take into account optimisim about future improvements in tooling. > So even if there were zero problems found so far, it is still a positive > change. > It is if you want to spin it that way. > I would agree if these changes were high risk, though; but they are > almost trivial. > This is trivial: case 1: this(); + fallthrough; case 2: that(); But what we inevitably get is changes like this: case 3: this(); + break; case 4: hmmm(); Why? Mainly to silence the compiler. Also because the patch author argued successfully that they had found a theoretical bug, often in mature code. But is anyone keeping score of the regressions? If unreported bugs count, what about unreported regressions? > Cheers, > Miguel >
Re: [RFC] MAINTAINERS tag for cleanup robot
On Sun, 22 Nov 2020, Joe Perches wrote: > On Sun, 2020-11-22 at 08:49 -0800, James Bottomley wrote: > > We can enforce sysfs_emit going forwards > > using tools like checkpatch > > It's not really possible for checkpatch to find or warn about > sysfs uses of sprintf. checkpatch is really just a trivial > line-by-line parser and it has no concept of code intent. > Checkpatch does suffer from the limitations of regular expressions. But that doesn't stop people from using it. Besides, Coccinelle can do analyses that can't be done with regular expressions, so it's moot. > It just can't warn on every use of the sprintf family. > There are just too many perfectly valid uses. > > > but there's no benefit and a lot of harm to > > be done by trying to churn the entire tree > > Single uses of sprintf for sysfs is not really any problem. > > But likely there are still several possible overrun sprintf/snprintf > paths in sysfs. Some of them are very obscure and unlikely to be > found by a robot as the logic for sysfs buf uses can be fairly twisty. > Logic errors of this kind are susceptible to fuzzing, formal methods, symbolic execution etc. No doubt there are other techniques that I don't know about. > But provably correct conversions IMO _should_ be done and IMO churn > considerations should generally have less importance. > Provably equivalent conversions are provably churn. So apparently you're advocating changes that are not provably equivalent. These are patches for code not that's not been shown to be buggy. Code which, after patching, can be shown to be free of a specific kind of theoretical bug. Hardly "provably correct". The problem is, the class of theoretical bugs that can be avoided in this way is probably limitless, as is the review cost and the risk of accidental regressions. And the payoff is entirely theoretical. Moreover, the patch review workload for skilled humans is being generated by the automation, which is completely backwards: the machine is supposed to be helping.
[PATCH v2] m68k: Fix WARNING splat in pmac_zilog driver
Don't add platform resources that won't be used. This avoids a
recently-added warning from the driver core, that can show up on a
multi-platform kernel when !MACH_IS_MAC.
[ cut here ]
WARNING: CPU: 0 PID: 0 at drivers/base/platform.c:224
platform_get_irq_optional+0x8e/0xce
0 is an invalid IRQ number
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 5.9.0-multi #1
Stack from 004b3f04:
004b3f04 00462c2f 00462c2f 004b3f20 0002e128 004754db 004b6ad4 004b3f4c
0002e19c 004754f7 00e0 00285ba0 0009 004b3f44
004754db 004b3f64 004b3f74 00285ba0 004754f7 00e0 0009 004754db
004fdf0c 005269e2 004fdf0c 004b3f88 00285cae 004b6964
004fdf0c 004b3fac 0051cc68 004b6964 004b6964 0200
0051cc3e 0023c18a 004b3fc0 0051cd8a 004fdf0c 0002 0052b43c 004b3fc8
Call Trace: [<0002e128>] __warn+0xa6/0xd6
[<0002e19c>] warn_slowpath_fmt+0x44/0x76
[<00285ba0>] platform_get_irq_optional+0x8e/0xce
[<00285ba0>] platform_get_irq_optional+0x8e/0xce
[<00285cae>] platform_get_irq+0x12/0x4c
[<0051cc68>] pmz_init_port+0x2a/0xa6
[<0051cc3e>] pmz_init_port+0x0/0xa6
[<0023c18a>] strlen+0x0/0x22
[<0051cd8a>] pmz_probe+0x34/0x88
[<0051cde6>] pmz_console_init+0x8/0x28
[<00511776>] console_init+0x1e/0x28
[<0005a3bc>] printk+0x0/0x16
[<0050a8a6>] start_kernel+0x368/0x4ce
[<005094f8>] _sinittext+0x4f8/0xc48
random: get_random_bytes called from print_oops_end_marker+0x56/0x80 with
crng_init=0
---[ end trace 392d8e82eed68d6c ]---
Commit a85a6c86c25b ("driver core: platform: Clarify that IRQ 0 is invalid"),
which introduced the WARNING, suggests that testing for irq == 0 is
undesirable. Instead of that comparison, just test for resource existence.
Cc: Michael Ellerman
Cc: Benjamin Herrenschmidt
Cc: Paul Mackerras
Cc: Joshua Thompson
Cc: Greg Kroah-Hartman
Cc: Jiri Slaby
Cc: sta...@vger.kernel.org # v5.8+
References: commit a85a6c86c25b ("driver core: platform: Clarify that IRQ 0 is
invalid")
Reported-by: Laurent Vivier
Signed-off-by: Finn Thain
---
Changed since v1:
- Add a comment to explain the need for the global structs.
- Expand the commit log to better explain the intention of the patch.
---
arch/m68k/mac/config.c | 17 +
drivers/tty/serial/pmac_zilog.c | 14 +-
2 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
index 0ac53d87493c..2bea1799b8de 100644
--- a/arch/m68k/mac/config.c
+++ b/arch/m68k/mac/config.c
@@ -777,16 +777,12 @@ static struct resource scc_b_rsrcs[] = {
struct platform_device scc_a_pdev = {
.name = "scc",
.id = 0,
- .num_resources = ARRAY_SIZE(scc_a_rsrcs),
- .resource = scc_a_rsrcs,
};
EXPORT_SYMBOL(scc_a_pdev);
struct platform_device scc_b_pdev = {
.name = "scc",
.id = 1,
- .num_resources = ARRAY_SIZE(scc_b_rsrcs),
- .resource = scc_b_rsrcs,
};
EXPORT_SYMBOL(scc_b_pdev);
@@ -813,10 +809,15 @@ static void __init mac_identify(void)
/* Set up serial port resources for the console initcall. */
- scc_a_rsrcs[0].start = (resource_size_t) mac_bi_data.sccbase + 2;
- scc_a_rsrcs[0].end = scc_a_rsrcs[0].start;
- scc_b_rsrcs[0].start = (resource_size_t) mac_bi_data.sccbase;
- scc_b_rsrcs[0].end = scc_b_rsrcs[0].start;
+ scc_a_rsrcs[0].start = (resource_size_t)mac_bi_data.sccbase + 2;
+ scc_a_rsrcs[0].end = scc_a_rsrcs[0].start;
+ scc_a_pdev.num_resources = ARRAY_SIZE(scc_a_rsrcs);
+ scc_a_pdev.resource = scc_a_rsrcs;
+
+ scc_b_rsrcs[0].start = (resource_size_t)mac_bi_data.sccbase;
+ scc_b_rsrcs[0].end = scc_b_rsrcs[0].start;
+ scc_b_pdev.num_resources = ARRAY_SIZE(scc_b_rsrcs);
+ scc_b_pdev.resource = scc_b_rsrcs;
switch (macintosh_config->scc_type) {
case MAC_SCC_PSC:
diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
index 96e7aa479961..216b75ef5048 100644
--- a/drivers/tty/serial/pmac_zilog.c
+++ b/drivers/tty/serial/pmac_zilog.c
@@ -1693,22 +1693,26 @@ static int __init pmz_probe(void)
#else
+/* On PCI PowerMacs, pmz_probe() does an explicit search of the OpenFirmware
+ * tree to obtain the device_nodes needed to start the console before the
+ * macio driver. On Macs without OpenFirmware, global platform_devices take
+ * the place of those device_nodes.
+ */
extern struct platform_device scc_a_pdev, scc_b_pdev;
static int __init pmz_init_port(struct uart_pmac_port *uap)
{
- struct resource *r_ports;
- int irq;
+ struct resource *r_ports, *r_irq;
r_ports = platform_get_resource(uap->pdev, IORESOURCE_MEM, 0
Re: [PATCH] m68k: Fix WARNING splat in pmac_zilog driver
On Fri, 20 Nov 2020, Geert Uytterhoeven wrote:
> Hi Finn,
>
> On Fri, Nov 20, 2020 at 5:51 AM Finn Thain wrote:
> > Don't add platform resources that won't be used. This avoids a
> > recently-added warning from the driver core, that can show up on a
> > multi-platform kernel when !MACH_IS_MAC.
> >
> > [ cut here ]
> > WARNING: CPU: 0 PID: 0 at drivers/base/platform.c:224
> > platform_get_irq_optional+0x8e/0xce
> > 0 is an invalid IRQ number
> > Modules linked in:
> > CPU: 0 PID: 0 Comm: swapper Not tainted 5.9.0-multi #1
> > Stack from 004b3f04:
> > 004b3f04 00462c2f 00462c2f 004b3f20 0002e128 004754db 004b6ad4
> > 004b3f4c
> > 0002e19c 004754f7 00e0 00285ba0 0009 004b3f44
> >
> > 004754db 004b3f64 004b3f74 00285ba0 004754f7 00e0 0009
> > 004754db
> > 004fdf0c 005269e2 004fdf0c 004b3f88 00285cae 004b6964
> >
> > 004fdf0c 004b3fac 0051cc68 004b6964 004b6964 0200
> >
> > 0051cc3e 0023c18a 004b3fc0 0051cd8a 004fdf0c 0002 0052b43c
> > 004b3fc8
> > Call Trace: [<0002e128>] __warn+0xa6/0xd6
> > [<0002e19c>] warn_slowpath_fmt+0x44/0x76
> > [<00285ba0>] platform_get_irq_optional+0x8e/0xce
> > [<00285ba0>] platform_get_irq_optional+0x8e/0xce
> > [<00285cae>] platform_get_irq+0x12/0x4c
> > [<0051cc68>] pmz_init_port+0x2a/0xa6
> > [<0051cc3e>] pmz_init_port+0x0/0xa6
> > [<0023c18a>] strlen+0x0/0x22
> > [<0051cd8a>] pmz_probe+0x34/0x88
> > [<0051cde6>] pmz_console_init+0x8/0x28
> > [<00511776>] console_init+0x1e/0x28
> > [<0005a3bc>] printk+0x0/0x16
> > [<0050a8a6>] start_kernel+0x368/0x4ce
> > [<005094f8>] _sinittext+0x4f8/0xc48
> > random: get_random_bytes called from print_oops_end_marker+0x56/0x80 with
> > crng_init=0
> > ---[ end trace 392d8e82eed68d6c ]---
> >
> > Cc: Michael Ellerman
> > Cc: Benjamin Herrenschmidt
> > Cc: Paul Mackerras
> > Cc: Joshua Thompson
> > Cc: Greg Kroah-Hartman
> > Cc: Jiri Slaby
> > Cc: sta...@vger.kernel.org # v5.8+
> > References: commit a85a6c86c25b ("driver core: platform: Clarify that IRQ 0
> > is invalid")
> > Reported-by: Laurent Vivier
> > Signed-off-by: Finn Thain
> > ---
> > The global platform_device structs provide the equivalent of a direct
> > search of the OpenFirmware tree, for platforms that don't have OF.
> > The purpose of that search is discussed in the comments in pmac_zilog.c:
> >
> > * First, we need to do a direct OF-based probe pass. We
> > * do that because we want serial console up before the
> > * macio stuffs calls us back
> >
> > The actual platform bus matching takes place later, with a module_initcall,
> > following the usual pattern.
>
> I think it would be good for this explanation to be part of the
> actual patch description above.
>
Thanks for your review.
I take that explanation as read because it was fundamental to the changes
I made to pmac_zilog.c back in 2009 with commit ec9cbe09899e ("pmac-zilog:
add platform driver").
IMO, being that it isn't news, it doesn't belong in the changelog.
However, I agree that it needs to be documented. How about I add a comment
to pmac_zilog.c?
> > --- a/arch/m68k/mac/config.c
> > +++ b/arch/m68k/mac/config.c
> > @@ -777,16 +777,12 @@ static struct resource scc_b_rsrcs[] = {
> > struct platform_device scc_a_pdev = {
> > .name = "scc",
> > .id = 0,
> > - .num_resources = ARRAY_SIZE(scc_a_rsrcs),
> > - .resource = scc_a_rsrcs,
> > };
> > EXPORT_SYMBOL(scc_a_pdev);
> >
> > struct platform_device scc_b_pdev = {
> > .name = "scc",
> > .id = 1,
> > - .num_resources = ARRAY_SIZE(scc_b_rsrcs),
> > - .resource = scc_b_rsrcs,
> > };
> > EXPORT_SYMBOL(scc_b_pdev);
> >
> > @@ -813,10 +809,15 @@ static void __init mac_identify(void)
> >
> > /* Set up serial port resources for the console initcall. */
> >
> > - scc_a_rsrcs[0].start = (resource_size_t) mac_bi_data.sccbase + 2;
> > - scc_a_rsrcs[0].end = scc_a_rsrcs[0].start;
> > - scc_b_rsrcs[0].start = (resource_size_t) mac_bi_data.sccbase;
> > - scc_b_rsrcs[0].end = scc_b_rsrcs[0].start;
> > + scc_a_rsrcs[0].start
[PATCH RESEND] ide/falconide: Fix module unload
Unloading the falconide module results in a crash:
Unable to handle kernel NULL pointer dereference at virtual address
Oops:
Modules linked in: falconide(-)
PC: [<002930b2>] ide_host_remove+0x2e/0x1d2
SR: 2000 SP: 00b49e28 a2: 009b0f90
d0: d1: 009b0f90d2: d3: 00b48000
d4: 003cef32d5: 00299188a0: 0086d000a1: 0086d000
Process rmmod (pid: 322, task=009b0f90)
Frame format=7 eff addr= ssw=0505 faddr=
wb 1 stat/addr/data:
wb 2 stat/addr/data:
wb 3 stat/addr/data: 00018da9
push data:
Stack from 00b49e90:
004c456a 0027f176 0027cb0a 0027cb9e 0086d00a 2187d3f0 0027f0e0
00b49ebc 2187d1f6 00b49ec8 002811e8 0086d000 00b49ef0 0028024c
0086d00a 002800d6 00279a1a 0001 0001 0086d00a 2187d3f0 00279a58
00b49f1c 002802e0 0086d00a 2187d3f0 004c456a 0086d00a ef96af74
2187d3f0 002805d2 800de064 00b49f44 0027f088 2187d3f0 00ac1cf4 2187d3f0
004c43be 2187d3f0 2187d3f0 800b66a8 00b49f5c 00280776 2187d3f0
Call Trace: [<0027f176>] __device_driver_unlock+0x0/0x48
[<0027cb0a>] device_links_busy+0x0/0x94
[<0027cb9e>] device_links_unbind_consumers+0x0/0x130
[<0027f0e0>] __device_driver_lock+0x0/0x5a
[<2187d1f6>] falconide_remove+0x12/0x18 [falconide]
[<002811e8>] platform_drv_remove+0x1c/0x28
[<0028024c>] device_release_driver_internal+0x176/0x17c
[<002800d6>] device_release_driver_internal+0x0/0x17c
[<00279a1a>] get_device+0x0/0x22
[<00279a58>] put_device+0x0/0x18
[<002802e0>] driver_detach+0x56/0x82
[<002805d2>] driver_remove_file+0x0/0x24
[<0027f088>] bus_remove_driver+0x4c/0xa4
[<00280776>] driver_unregister+0x28/0x5a
[<00281a00>] platform_driver_unregister+0x12/0x18
[<2187d2a0>] ide_falcon_driver_exit+0x10/0x16 [falconide]
[<000764f0>] sys_delete_module+0x110/0x1f2
[<000e83ea>] sys_rename+0x1a/0x1e
[<2e0c>] syscall+0x8/0xc
[<00188004>] ext4_multi_mount_protect+0x35a/0x3ce
Code: 0029 9188 4bf9 0027 aa1c 283c 003c ef32 <265c> 4a8b 6700 00b8 2043 2028
000c 0280 00ff ff00 6600 0176 40c0 7202 b2b9 004c
Disabling lock debugging due to kernel taint
This happens because the driver_data pointer is uninitialized.
Add the missing platform_set_drvdata() call. For clarity, use the
matching platform_get_drvdata() as well.
Cc: Michael Schmitz
Cc: Bartlomiej Zolnierkiewicz
Fixes: 5ed0794cde593 ("m68k/atari: Convert Falcon IDE drivers to platform
drivers")
Reviewed-by: Geert Uytterhoeven
Reviewed-by: Michael Schmitz
Signed-off-by: Finn Thain
---
This patch was tested using Aranym.
---
drivers/ide/falconide.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/ide/falconide.c b/drivers/ide/falconide.c
index dbeb2605e5f6..607c44bc50f1 100644
--- a/drivers/ide/falconide.c
+++ b/drivers/ide/falconide.c
@@ -166,6 +166,7 @@ static int __init falconide_init(struct platform_device
*pdev)
if (rc)
goto err_free;
+ platform_set_drvdata(pdev, host);
return 0;
err_free:
ide_host_free(host);
@@ -176,7 +177,7 @@ static int __init falconide_init(struct platform_device
*pdev)
static int falconide_remove(struct platform_device *pdev)
{
- struct ide_host *host = dev_get_drvdata(>dev);
+ struct ide_host *host = platform_get_drvdata(pdev);
ide_host_remove(host);
--
2.26.2
[PATCH] m68k/mac: Update Kconfig help
There is still some missing hardware support that affects all models, such as sound chip and localtalk support. However, many models are well supported, including the Quadra 800 emulated by QEMU. Missing hardware support is mostly documented at the web site, so add the URL. Cc: Joshua Thompson Signed-off-by: Finn Thain --- arch/m68k/Kconfig.machine | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/arch/m68k/Kconfig.machine b/arch/m68k/Kconfig.machine index 17e8c3a292d7..054ff6392329 100644 --- a/arch/m68k/Kconfig.machine +++ b/arch/m68k/Kconfig.machine @@ -30,11 +30,9 @@ config MAC select HAVE_ARCH_NVRAM_OPS help This option enables support for the Apple Macintosh series of - computers (yes, there is experimental support now, at least for part - of the series). - - Say N unless you're willing to code the remaining necessary support. - ;) + computers. If you plan to use this kernel on a Mac, say Y here and + browse the documentation available at <http://www.mac.linux-m68k.org/>; + otherwise say N. config APOLLO bool "Apollo support" -- 2.26.2
[PATCH] m68k/mac: Remove dead code
Cc: Joshua Thompson
Signed-off-by: Finn Thain
---
arch/m68k/mac/via.c | 15 ---
1 file changed, 15 deletions(-)
diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
index 66209cb7696f..4226ae2e7501 100644
--- a/arch/m68k/mac/via.c
+++ b/arch/m68k/mac/via.c
@@ -305,21 +305,6 @@ void via_l2_flush(int writeback)
local_irq_restore(flags);
}
-/*
- * Return the status of the L2 cache on a IIci
- */
-
-int via_get_cache_disable(void)
-{
- /* Safeguard against being called accidentally */
- if (!via2) {
- printk(KERN_ERR "via_get_cache_disable called on a non-VIA
machine!\n");
- return 1;
- }
-
- return (int) via2[gBufB] & VIA2B_vCDis;
-}
-
/*
* Initialize VIA2 for Nubus access
*/
--
2.26.2
[PATCH] m68k/mac: Remove redundant VIA register writes
There's no need to write the same value to the timer latch and timer
counter registers. Values written to the counter registers get stored
in the latches anyway. The write to vT1CH copies the latch values to
the counter.
Cc: Joshua Thompson
Tested-by: Stan Johnson
Signed-off-by: Finn Thain
---
arch/m68k/mac/via.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
index 4226ae2e7501..36ff997d4706 100644
--- a/arch/m68k/mac/via.c
+++ b/arch/m68k/mac/via.c
@@ -170,8 +170,6 @@ void __init via_init(void)
via1[vIER] = 0x7F;
via1[vIFR] = 0x7F;
- via1[vT1LL] = 0;
- via1[vT1LH] = 0;
via1[vT1CL] = 0;
via1[vT1CH] = 0;
via1[vT2CL] = 0;
@@ -226,8 +224,6 @@ void __init via_init(void)
via2[gIER] = 0x7F;
via2[gIFR] = 0x7F | rbv_clear;
if (!rbv_present) {
- via2[vT1LL] = 0;
- via2[vT1LH] = 0;
via2[vT1CL] = 0;
via2[vT1CH] = 0;
via2[vT2CL] = 0;
@@ -605,8 +601,6 @@ void __init via_init_clock(irq_handler_t timer_routine)
return;
}
- via1[vT1LL] = VIA_TC_LOW;
- via1[vT1LH] = VIA_TC_HIGH;
via1[vT1CL] = VIA_TC_LOW;
via1[vT1CH] = VIA_TC_HIGH;
via1[vACR] |= 0x40;
--
2.26.2
[PATCH] macintosh/adb-iop: Always wait for reply message from IOP
A recent patch incorrectly altered the adb-iop state machine behaviour
and introduced a regression that can appear intermittently as a
malfunctioning ADB input device. This seems to be caused when reply
packets from different ADB commands become mixed up, especially during
the adb bus scan. Fix this by unconditionally entering the awaiting_reply
state after sending an explicit command, even when the ADB command won't
generate a reply from the ADB device.
Cc: Joshua Thompson
Fixes: e2954e5f727f ("macintosh/adb-iop: Implement sending -> idle state
transition")
Tested-by: Stan Johnson
Signed-off-by: Finn Thain
---
drivers/macintosh/adb-iop.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/macintosh/adb-iop.c b/drivers/macintosh/adb-iop.c
index 6b26b6a2c463..0ee327249150 100644
--- a/drivers/macintosh/adb-iop.c
+++ b/drivers/macintosh/adb-iop.c
@@ -84,10 +84,7 @@ static void adb_iop_complete(struct iop_msg *msg)
local_irq_save(flags);
- if (current_req->reply_expected)
- adb_iop_state = awaiting_reply;
- else
- adb_iop_done();
+ adb_iop_state = awaiting_reply;
local_irq_restore(flags);
}
@@ -95,8 +92,9 @@ static void adb_iop_complete(struct iop_msg *msg)
/*
* Listen for ADB messages from the IOP.
*
- * This will be called when unsolicited messages (usually replies to TALK
- * commands or autopoll packets) are received.
+ * This will be called when unsolicited IOP messages are received.
+ * These IOP messages can carry ADB autopoll responses and also occur
+ * after explicit ADB commands.
*/
static void adb_iop_listen(struct iop_msg *msg)
@@ -123,8 +121,10 @@ static void adb_iop_listen(struct iop_msg *msg)
if (adb_iop_state == awaiting_reply) {
struct adb_request *req = current_req;
- req->reply_len = amsg->count + 1;
- memcpy(req->reply, >cmd, req->reply_len);
+ if (req->reply_expected) {
+ req->reply_len = amsg->count + 1;
+ memcpy(req->reply, >cmd, req->reply_len);
+ }
req_done = true;
}
--
2.26.2
[PATCH] m68k/mac: Refactor iop_preinit() and iop_init()
The idea behind iop_preinit() was to put the SCC IOP into bypass mode.
However, that remains unimplemented and implementing it would be
difficult. Let the comments and code reflect this. Even if iop_preinit()
worked as described in the comments, it gets called immediately before
iop_init() so it might as well part of iop_init().
Cc: Joshua Thompson
Tested-by: Stan Johnson
Signed-off-by: Finn Thain
---
arch/m68k/mac/config.c | 8 ---
arch/m68k/mac/iop.c| 54 ++
2 files changed, 18 insertions(+), 44 deletions(-)
diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
index 2bea1799b8de..f66944909be9 100644
--- a/arch/m68k/mac/config.c
+++ b/arch/m68k/mac/config.c
@@ -55,7 +55,6 @@ struct mac_booter_data mac_bi_data;
static unsigned long mac_orig_videoaddr;
extern int mac_hwclk(int, struct rtc_time *);
-extern void iop_preinit(void);
extern void iop_init(void);
extern void via_init(void);
extern void via_init_clock(irq_handler_t func);
@@ -836,13 +835,6 @@ static void __init mac_identify(void)
break;
}
- /*
-* We need to pre-init the IOPs, if any. Otherwise
-* the serial console won't work if the user had
-* the serial ports set to "Faster" mode in MacOS.
-*/
- iop_preinit();
-
pr_info("Detected Macintosh model: %d\n", model);
/*
diff --git a/arch/m68k/mac/iop.c b/arch/m68k/mac/iop.c
index c669a7644301..de156a027f5b 100644
--- a/arch/m68k/mac/iop.c
+++ b/arch/m68k/mac/iop.c
@@ -47,6 +47,10 @@
*
* TODO:
*
+ * o The SCC IOP has to be placed in bypass mode before the serial console
+ * gets initialized. iop_init() would be one place to do that. Or the
+ * bootloader could do that. For now, the Serial Switch control panel
+ * is needed for that -- contrary to the changelog above.
* o Something should be periodically checking iop_alive() to make sure the
* IOP hasn't died.
* o Some of the IOP manager routines need better error checking and
@@ -224,40 +228,6 @@ static struct iop_msg *iop_get_unused_msg(void)
return NULL;
}
-/*
- * This is called by the startup code before anything else. Its purpose
- * is to find and initialize the IOPs early in the boot sequence, so that
- * the serial IOP can be placed into bypass mode _before_ we try to
- * initialize the serial console.
- */
-
-void __init iop_preinit(void)
-{
- if (macintosh_config->scc_type == MAC_SCC_IOP) {
- if (macintosh_config->ident == MAC_MODEL_IIFX) {
- iop_base[IOP_NUM_SCC] = (struct mac_iop *)
SCC_IOP_BASE_IIFX;
- } else {
- iop_base[IOP_NUM_SCC] = (struct mac_iop *)
SCC_IOP_BASE_QUADRA;
- }
- iop_scc_present = 1;
- } else {
- iop_base[IOP_NUM_SCC] = NULL;
- iop_scc_present = 0;
- }
- if (macintosh_config->adb_type == MAC_ADB_IOP) {
- if (macintosh_config->ident == MAC_MODEL_IIFX) {
- iop_base[IOP_NUM_ISM] = (struct mac_iop *)
ISM_IOP_BASE_IIFX;
- } else {
- iop_base[IOP_NUM_ISM] = (struct mac_iop *)
ISM_IOP_BASE_QUADRA;
- }
- iop_stop(iop_base[IOP_NUM_ISM]);
- iop_ism_present = 1;
- } else {
- iop_base[IOP_NUM_ISM] = NULL;
- iop_ism_present = 0;
- }
-}
-
/*
* Initialize the IOPs, if present.
*/
@@ -266,11 +236,23 @@ void __init iop_init(void)
{
int i;
- if (iop_scc_present) {
+ if (macintosh_config->scc_type == MAC_SCC_IOP) {
+ if (macintosh_config->ident == MAC_MODEL_IIFX)
+ iop_base[IOP_NUM_SCC] = (struct mac_iop
*)SCC_IOP_BASE_IIFX;
+ else
+ iop_base[IOP_NUM_SCC] = (struct mac_iop
*)SCC_IOP_BASE_QUADRA;
+ iop_scc_present = 1;
pr_debug("SCC IOP detected at %p\n", iop_base[IOP_NUM_SCC]);
}
- if (iop_ism_present) {
+ if (macintosh_config->adb_type == MAC_ADB_IOP) {
+ if (macintosh_config->ident == MAC_MODEL_IIFX)
+ iop_base[IOP_NUM_ISM] = (struct mac_iop
*)ISM_IOP_BASE_IIFX;
+ else
+ iop_base[IOP_NUM_ISM] = (struct mac_iop
*)ISM_IOP_BASE_QUADRA;
+ iop_ism_present = 1;
pr_debug("ISM IOP detected at %p\n", iop_base[IOP_NUM_ISM]);
+
+ iop_stop(iop_base[IOP_NUM_ISM]);
iop_start(iop_base[IOP_NUM_ISM]);
iop_alive(iop_base[IOP_NUM_ISM]); /* clears the alive flag */
}
--
2.26.2
[PATCH] macintosh/adb-iop: Send correct poll command
The behaviour of the IOP firmware is not well documented but we do know
that IOP message reply data can be used to issue new ADB commands.
Use the message reply to better control autopoll behaviour by sending
a Talk Register 0 command after every ADB response, not unlike the
algorithm in the via-macii driver. This poll command is addressed to
that device which last received a Talk command (explicit or otherwise).
Cc: Joshua Thompson
Fixes: fa3b5a9929fc ("macintosh/adb-iop: Implement idle -> sending state
transition")
Tested-by: Stan Johnson
Signed-off-by: Finn Thain
---
drivers/macintosh/adb-iop.c | 40 +++--
1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/drivers/macintosh/adb-iop.c b/drivers/macintosh/adb-iop.c
index f3d1a460fbce..6b26b6a2c463 100644
--- a/drivers/macintosh/adb-iop.c
+++ b/drivers/macintosh/adb-iop.c
@@ -25,6 +25,7 @@
static struct adb_request *current_req;
static struct adb_request *last_req;
static unsigned int autopoll_devs;
+static u8 autopoll_addr;
static enum adb_iop_state {
idle,
@@ -41,6 +42,11 @@ static int adb_iop_autopoll(int);
static void adb_iop_poll(void);
static int adb_iop_reset_bus(void);
+/* ADB command byte structure */
+#define ADDR_MASK 0xF0
+#define OP_MASK 0x0C
+#define TALK0x0C
+
struct adb_driver adb_iop_driver = {
.name = "ISM IOP",
.probe= adb_iop_probe,
@@ -96,17 +102,24 @@ static void adb_iop_complete(struct iop_msg *msg)
static void adb_iop_listen(struct iop_msg *msg)
{
struct adb_iopmsg *amsg = (struct adb_iopmsg *)msg->message;
+ u8 addr = (amsg->cmd & ADDR_MASK) >> 4;
+ u8 op = amsg->cmd & OP_MASK;
unsigned long flags;
bool req_done = false;
local_irq_save(flags);
- /* Handle a timeout. Timeout packets seem to occur even after
-* we've gotten a valid reply to a TALK, presumably because of
-* autopolling.
+ /* Responses to Talk commands may be unsolicited as they are
+* produced when the IOP polls devices. They are mostly timeouts.
*/
-
- if (amsg->flags & ADB_IOP_EXPLICIT) {
+ if (op == TALK && ((1 << addr) & autopoll_devs))
+ autopoll_addr = addr;
+
+ switch (amsg->flags & (ADB_IOP_EXPLICIT |
+ ADB_IOP_AUTOPOLL |
+ ADB_IOP_TIMEOUT)) {
+ case ADB_IOP_EXPLICIT:
+ case ADB_IOP_EXPLICIT | ADB_IOP_TIMEOUT:
if (adb_iop_state == awaiting_reply) {
struct adb_request *req = current_req;
@@ -115,12 +128,16 @@ static void adb_iop_listen(struct iop_msg *msg)
req_done = true;
}
- } else if (!(amsg->flags & ADB_IOP_TIMEOUT)) {
- adb_input(>cmd, amsg->count + 1,
- amsg->flags & ADB_IOP_AUTOPOLL);
+ break;
+ case ADB_IOP_AUTOPOLL:
+ if (((1 << addr) & autopoll_devs) &&
+ amsg->cmd == ADB_READREG(addr, 0))
+ adb_input(>cmd, amsg->count + 1, 1);
+ break;
}
-
- msg->reply[0] = autopoll_devs ? ADB_IOP_AUTOPOLL : 0;
+ msg->reply[0] = autopoll_addr ? ADB_IOP_AUTOPOLL : 0;
+ msg->reply[1] = 0;
+ msg->reply[2] = autopoll_addr ? ADB_READREG(autopoll_addr, 0) : 0;
iop_complete_message(msg);
if (req_done)
@@ -233,6 +250,9 @@ static void adb_iop_set_ap_complete(struct iop_msg *msg)
struct adb_iopmsg *amsg = (struct adb_iopmsg *)msg->message;
autopoll_devs = (amsg->data[1] << 8) | amsg->data[0];
+ if (autopoll_devs & (1 << autopoll_addr))
+ return;
+ autopoll_addr = autopoll_devs ? (ffs(autopoll_devs) - 1) : 0;
}
static int adb_iop_autopoll(int devs)
--
2.26.2
[PATCH] scsi/atari_scsi: Fix race condition between .queuecommand and EH
It is possible that bus_reset_cleanup() or .eh_abort_handler could
be invoked during NCR5380_queuecommand(). If that takes place before
the new command is enqueued and after the ST-DMA "lock" has been
acquired, the ST-DMA "lock" will be released again. This will result
in a lost DMA interrupt and a command timeout. Fix this by excluding
EH and interrupt handlers while the new command is enqueued.
Signed-off-by: Finn Thain
---
Michael, would you please send your Acked-by or Reviewed-and-tested-by?
These two patches taken together should be equivalent to the one you tested
recently. I've split it into two as that seemed to make more sense.
---
drivers/scsi/NCR5380.c| 9 ++---
drivers/scsi/atari_scsi.c | 10 +++---
2 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index d654a6cc4162..ea4b5749e7da 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -580,11 +580,14 @@ static int NCR5380_queue_command(struct Scsi_Host
*instance,
cmd->result = 0;
- if (!NCR5380_acquire_dma_irq(instance))
- return SCSI_MLQUEUE_HOST_BUSY;
-
spin_lock_irqsave(>lock, flags);
+ if (!NCR5380_acquire_dma_irq(instance)) {
+ spin_unlock_irqrestore(>lock, flags);
+
+ return SCSI_MLQUEUE_HOST_BUSY;
+ }
+
/*
* Insert the cmd into the issue queue. Note that REQUEST SENSE
* commands are added to the head of the queue since any command will
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index a82b63a66635..95d7a3586083 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -376,15 +376,11 @@ static int falcon_get_lock(struct Scsi_Host *instance)
if (IS_A_TT())
return 1;
- if (stdma_is_locked_by(scsi_falcon_intr) &&
- instance->hostt->can_queue > 1)
+ if (stdma_is_locked_by(scsi_falcon_intr))
return 1;
- if (in_interrupt())
- return stdma_try_lock(scsi_falcon_intr, instance);
-
- stdma_lock(scsi_falcon_intr, instance);
- return 1;
+ /* stdma_lock() may sleep which means it can't be used here */
+ return stdma_try_lock(scsi_falcon_intr, instance);
}
#ifndef MODULE
--
2.26.2
[PATCH] m68k: Fix WARNING splat in pmac_zilog driver
Don't add platform resources that won't be used. This avoids a
recently-added warning from the driver core, that can show up on a
multi-platform kernel when !MACH_IS_MAC.
[ cut here ]
WARNING: CPU: 0 PID: 0 at drivers/base/platform.c:224
platform_get_irq_optional+0x8e/0xce
0 is an invalid IRQ number
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 5.9.0-multi #1
Stack from 004b3f04:
004b3f04 00462c2f 00462c2f 004b3f20 0002e128 004754db 004b6ad4 004b3f4c
0002e19c 004754f7 00e0 00285ba0 0009 004b3f44
004754db 004b3f64 004b3f74 00285ba0 004754f7 00e0 0009 004754db
004fdf0c 005269e2 004fdf0c 004b3f88 00285cae 004b6964
004fdf0c 004b3fac 0051cc68 004b6964 004b6964 0200
0051cc3e 0023c18a 004b3fc0 0051cd8a 004fdf0c 0002 0052b43c 004b3fc8
Call Trace: [<0002e128>] __warn+0xa6/0xd6
[<0002e19c>] warn_slowpath_fmt+0x44/0x76
[<00285ba0>] platform_get_irq_optional+0x8e/0xce
[<00285ba0>] platform_get_irq_optional+0x8e/0xce
[<00285cae>] platform_get_irq+0x12/0x4c
[<0051cc68>] pmz_init_port+0x2a/0xa6
[<0051cc3e>] pmz_init_port+0x0/0xa6
[<0023c18a>] strlen+0x0/0x22
[<0051cd8a>] pmz_probe+0x34/0x88
[<0051cde6>] pmz_console_init+0x8/0x28
[<00511776>] console_init+0x1e/0x28
[<0005a3bc>] printk+0x0/0x16
[<0050a8a6>] start_kernel+0x368/0x4ce
[<005094f8>] _sinittext+0x4f8/0xc48
random: get_random_bytes called from print_oops_end_marker+0x56/0x80 with
crng_init=0
---[ end trace 392d8e82eed68d6c ]---
Cc: Michael Ellerman
Cc: Benjamin Herrenschmidt
Cc: Paul Mackerras
Cc: Joshua Thompson
Cc: Greg Kroah-Hartman
Cc: Jiri Slaby
Cc: sta...@vger.kernel.org # v5.8+
References: commit a85a6c86c25b ("driver core: platform: Clarify that IRQ 0 is
invalid")
Reported-by: Laurent Vivier
Signed-off-by: Finn Thain
---
The global platform_device structs provide the equivalent of a direct
search of the OpenFirmware tree, for platforms that don't have OF.
The purpose of that search is discussed in the comments in pmac_zilog.c:
* First, we need to do a direct OF-based probe pass. We
* do that because we want serial console up before the
* macio stuffs calls us back
The actual platform bus matching takes place later, with a module_initcall,
following the usual pattern.
---
arch/m68k/mac/config.c | 17 +
drivers/tty/serial/pmac_zilog.c | 9 -
2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
index 0ac53d87493c..2bea1799b8de 100644
--- a/arch/m68k/mac/config.c
+++ b/arch/m68k/mac/config.c
@@ -777,16 +777,12 @@ static struct resource scc_b_rsrcs[] = {
struct platform_device scc_a_pdev = {
.name = "scc",
.id = 0,
- .num_resources = ARRAY_SIZE(scc_a_rsrcs),
- .resource = scc_a_rsrcs,
};
EXPORT_SYMBOL(scc_a_pdev);
struct platform_device scc_b_pdev = {
.name = "scc",
.id = 1,
- .num_resources = ARRAY_SIZE(scc_b_rsrcs),
- .resource = scc_b_rsrcs,
};
EXPORT_SYMBOL(scc_b_pdev);
@@ -813,10 +809,15 @@ static void __init mac_identify(void)
/* Set up serial port resources for the console initcall. */
- scc_a_rsrcs[0].start = (resource_size_t) mac_bi_data.sccbase + 2;
- scc_a_rsrcs[0].end = scc_a_rsrcs[0].start;
- scc_b_rsrcs[0].start = (resource_size_t) mac_bi_data.sccbase;
- scc_b_rsrcs[0].end = scc_b_rsrcs[0].start;
+ scc_a_rsrcs[0].start = (resource_size_t)mac_bi_data.sccbase + 2;
+ scc_a_rsrcs[0].end = scc_a_rsrcs[0].start;
+ scc_a_pdev.num_resources = ARRAY_SIZE(scc_a_rsrcs);
+ scc_a_pdev.resource = scc_a_rsrcs;
+
+ scc_b_rsrcs[0].start = (resource_size_t)mac_bi_data.sccbase;
+ scc_b_rsrcs[0].end = scc_b_rsrcs[0].start;
+ scc_b_pdev.num_resources = ARRAY_SIZE(scc_b_rsrcs);
+ scc_b_pdev.resource = scc_b_rsrcs;
switch (macintosh_config->scc_type) {
case MAC_SCC_PSC:
diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
index 96e7aa479961..95abdb305d67 100644
--- a/drivers/tty/serial/pmac_zilog.c
+++ b/drivers/tty/serial/pmac_zilog.c
@@ -1697,18 +1697,17 @@ extern struct platform_device scc_a_pdev, scc_b_pdev;
static int __init pmz_init_port(struct uart_pmac_port *uap)
{
- struct resource *r_ports;
- int irq;
+ struct resource *r_ports, *r_irq;
r_ports = platform_get_resource(uap->pdev, IORESOURCE_MEM, 0);
- irq = platform_get_irq(uap->pdev, 0);
- if (!r_ports || irq <= 0)
+ r_irq = platform_get_resource(uap->pdev, IORESOURCE_IRQ, 0);
+ if (!r_ports || !r_irq)
return -ENODEV;
[PATCH] scsi/NCR5380: Reduce NCR5380_maybe_release_dma_irq() call sites
Refactor to avoid needless calls to NCR5380_maybe_release_dma_irq().
This makes the machine code smaller and the source more readable.
Signed-off-by: Finn Thain
---
drivers/scsi/NCR5380.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index ea4b5749e7da..d597d7493a62 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -725,7 +725,6 @@ static void NCR5380_main(struct work_struct *work)
if (!NCR5380_select(instance, cmd)) {
dsprintk(NDEBUG_MAIN, instance, "main: select
complete\n");
- maybe_release_dma_irq(instance);
} else {
dsprintk(NDEBUG_MAIN | NDEBUG_QUEUES, instance,
"main: select failed, returning %p to
queue\n", cmd);
@@ -737,8 +736,10 @@ static void NCR5380_main(struct work_struct *work)
NCR5380_information_transfer(instance);
done = 0;
}
- if (!hostdata->connected)
+ if (!hostdata->connected) {
NCR5380_write(SELECT_ENABLE_REG, hostdata->id_mask);
+ maybe_release_dma_irq(instance);
+ }
spin_unlock_irq(>lock);
if (!done)
cond_resched();
@@ -1844,7 +1845,6 @@ static void NCR5380_information_transfer(struct Scsi_Host
*instance)
*/
NCR5380_write(TARGET_COMMAND_REG, 0);
- maybe_release_dma_irq(instance);
return;
case MESSAGE_REJECT:
/* Accept message by clearing ACK */
@@ -1976,7 +1976,6 @@ static void NCR5380_information_transfer(struct Scsi_Host
*instance)
hostdata->busy[scmd_id(cmd)] &= ~(1 <<
cmd->device->lun);
cmd->result = DID_ERROR << 16;
complete_cmd(instance, cmd);
- maybe_release_dma_irq(instance);
return;
}
msgout = NOP;
@@ -2312,7 +2311,6 @@ static int NCR5380_abort(struct scsi_cmnd *cmd)
}
queue_work(hostdata->work_q, >main_task);
- maybe_release_dma_irq(instance);
spin_unlock_irqrestore(>lock, flags);
return result;
@@ -2368,7 +2366,6 @@ static void bus_reset_cleanup(struct Scsi_Host *instance)
hostdata->dma_len = 0;
queue_work(hostdata->work_q, >main_task);
- maybe_release_dma_irq(instance);
}
/**
--
2.26.2
Re: [RFC 13/13] m68k: mac: convert to generic clockevent
On Fri, 30 Oct 2020, Greg Ungerer wrote: > > > > > ... > > > > The other 11 platforms in that category also have 'synthetic' > > > > clocksources derived from a timer reload interrupt. In 3 cases, > > > > the clocksource read method does not (or can not) check for a > > > > pending counter reload interrupt. For these also, I see no > > > > practical alternative to the approach you've taken in your RFC > > > > patch: > > > > > > > > arch/m68k/68000/timers.c > > > > arch/m68k/atari/time.c > > > > arch/m68k/coldfire/timers.c > > > > > > Agreed. It's possible there is a way, but I don't see one either. > > > > > > > For arch/m68k/68000/timers.c, I suppose we may be able to check for > > the TMR1 bit in the Interrupt Status Register at 0xF30C or the > > COMP bit in the Timer Status Register at 0xF60A. But testing that > > patch could be difficult. > > > > I expect that equivalent flags are available in Coldfire registers, > > making it possible to improve upon mcftmr_read_clk() and > > m68328_read_clk() if need be -- that is, if it turns out that the > > clocksource interrupt was subject to higher priority IRQs that would > > slow down the clocksource or defeat its monotonicity. > > > > The other difficulty is a lack of hardware timers. There's only one > > timer on MC68EZ328. On Atari, for now only Timer C is available though > > Michael has said that it would be possible to free up Timer D. Some > > Coldfire chips have only 2 timers and the second timer seems to be > > allocated to profiling. > > FWIW, I would have no problem with ditching the profiling clock, and > using both on ColdFire platforms that have this. I doubt anyone has used > that profiling setup in a _very_ long time. > If we ditched the Coldfire profiling clock, it would be possible to dedicate one hardware timer to the clocksource device and the other to the (oneshot) clockevent device. That's a win if it means that the clocksource can use the full counter width (making timer interrupts less frequent and timer interrupt latency less problematic) and run at higher frequency (making the clocksource more precise). Also, note that hrtimers won't work with a periodic clockevent device (as in Arnd's RFC patch). If you want hrtimers, I think you'll need both hardware timers or else re-implement the existing synthetic clocksource using the same oneshot timer driving a new oneshot clockevent device. Please note that the lack of a spare hardware timer is a separate issue to the failure of mcftmr_read_clk() and m68328_read_clk() to check the timer reload interrupt flag (which may make those clocksources needlessly susceptible to issues caused by timer interrupt latency...).
Re: [RFC 13/13] m68k: mac: convert to generic clockevent
On Fri, 23 Oct 2020, Geert Uytterhoeven wrote: > > > > The arm/rpc timer seems to be roughly in the same category as most > > > > of the m68k ones or the i8253 counter on a PC. It's possible that > > > > some of them could use the same logic as > > > > drivers/clocksource/i8253.o as long as there is any hardware > > > > oneshot mode. > > > > > > There appear to be 15 platforms in that category. 4 have no > > > clocksource besides the jiffies clocksource, meaning there's no > > > practical alternative to using a periodic tick, like you did in your > > > RFC patch: > > > > > > arch/m68k/apollo/config.c > > > arch/m68k/q40/q40ints.c > > > arch/m68k/sun3/sun3ints.c > > > arch/m68k/sun3x/time.c > > > > Do any of these have users? I'm fairly sure sun3x has never worked in > > mainline, sun3 seems to still need the same few patches it did 20 > > years ago. I couldn't find much about Linux on Apollo or q40, the > > information on the web for either of them seems to all be for > > linux-2.4 kernels. > > They probably don't have any users. I have access to several Sun 3 machines but no time to work on that port, unfortunately. Are these 4 platforms (those with no clocksource besides the "jiffies" clocksource) the only reason for CONFIG_TIME_LOW_RES on m68k?
Re: [PATCH v2 00/15] timers: clean up ARCH_GETTIMEOFFSET, LEGACY_TIMER_TICK
On Fri, 30 Oct 2020, Arnd Bergmann wrote: > CONFIG_ARCH_GETTIMEOFFSET has been gradually phased out from all > platforms, with only ARM EBSA110 recently. As this has no more known > users, the first three patches remove EBSA110 along with one platform > specific driver and the ARCH_GETTIMEOFFSET infrastructure. > The CONFIG_ARCH_GETTIMEOFFSET patches weren't part of v1. Is there some kind of dependency here?
Re: [RFC 13/13] m68k: mac: convert to generic clockevent
On Fri, 23 Oct 2020, Arnd Bergmann wrote:
> On Sun, Oct 18, 2020 at 2:55 AM Finn Thain
> wrote:
> > On Thu, 15 Oct 2020, Arnd Bergmann wrote:
> > > On Thu, Oct 15, 2020 at 3:19 AM Finn Thain
> > > wrote:
> > > > On Sat, 10 Oct 2020, Arnd Bergmann wrote:
> >
> > That configuration still produces the same 5 KiB of bloat. I see that
> > kernel/time/Kconfig has this --
> >
> > # Core internal switch. Selected by NO_HZ_COMMON / HIGH_RES_TIMERS. This is
> > # only related to the tick functionality. Oneshot clockevent devices
> > # are supported independent of this.
> > config TICK_ONESHOT
> > bool
> >
> > But my question was really about both kinds of dead code (oneshot
> > device support and oneshot tick support). Anyway, after playing with
> > the code for a bit I don't see any easy way to reduce the growth in
> > text size.
>
> Did you look more deeply into where those 5KB are? Is this just the code
> in kernel/time/{clockevents,tick-common}.c and the added platform
> specific bits, or is there something more?
What I did was to list the relevant functions using scripts/bloat-o-meter
and tried stubbing out some code related to oneshot clockevent devices. I
didn't find any low fruit and don't plan to pursue that without the help
of LTO.
> I suppose the sysfs interface and the clockevents_update_freq() logic
> are not really needed on m68k, but it wouldn't make much sense to split
> those out either.
>
> How does the 5KB bloat compare to the average bloat we get from one
> release to the next? Geert has been collecting statistics for this.
>
Perhaps that 5 KB is justified by gaining the hrtimers feature... hard to
say; it's never been available on these platforms. I can see the value in
it though.
> > > Yes, makes sense. I think the one remaining problem with the
> > > periodic mode in this driver is that it can drop timer ticks when
> > > interrupts are disabled for too long, while in oneshot mode there
> > > may be a way to know how much time has passed since the last tick as
> > > long as the counter does not overflow.
> >
> > Is there any benefit from adopting a oneshot tick (rather than
> > periodic) if no clocksource is consulted when calculating the next
> > interval? (I'm assuming NO_HZ is not in use, for reasons discussed
> > below.)
>
> If the clocksource does not set CLOCK_SOURCE_IS_CONTINOUS, the kernel
> will keep using periodic timers and not allow hrtimers.
>
IIUC, when HIGH_RES_TIMERS=y, the kernel will enable hrtimers only if the
platform provides both a continuous clocksource device and a oneshot
clockevent device. However, the "jiffies" clocksource does not set
CLOCK_SOURCE_IS_CONTINOUS and neither does the one in
arch/arm/mach-rpc/time.c.
When HIGH_RES_TIMERS=n and NO_HZ_COMMON=n (which is presently the case for
all platforms with GENERIC_CLOCKEVENTS=n) there's no use for a oneshot
clockevent device, right?
It seems likely that NO_HZ_COMMON=n because the clocksources on these
platforms produce a periodic interrupt regardless (e.g.
clocksource/i8253.c, arm/rpc, m68k platform timers etc.).
Finally, HIGH_RES_TIMERS=n seems likely if the only clocksource is
unreliable (e.g. because it loses time due to interrupt priorities). There
may be a few of platforms in this category (Mac, Atari?).
> > > I would agree that despite this oneshot mode is probably worse
> > > overall for timekeeping if the register accesses introduce
> > > systematic errors.
> > >
> >
> > It probably has to be tried. But consulting a VIA clocksource on every
> > tick would be expensive on this platform, so if that was the only way
> > to avoid cumulative errors, I'd probably just stick with the periodic
> > tick.
>
> I'm sure there is a tradeoff somewhere. Without hrtimers, some events
> will take longer when they have to wait for the next tick, and using
> NO_HZ_FULL can help help make things faster on some workloads.
>
Yes, such a tradeoff is discussed in drivers/iio/adc/ep93xx_adc.c.
But OTOH, Documentation/timers/timers-howto.rst says,
On slower systems, (embedded, OR perhaps a speed-stepped PC!) the
overhead of setting up the hrtimers for usleep *may* not be worth it
I guess it has to be tried.
> ...
> > The other 11 platforms in that category also have 'synthetic'
> > clocksources derived from a timer reload interrupt. In 3 cases, the
> > clocksource read method does not (or can not) check for a pending
> > counter reload interrupt. For these also, I see no practical
> > alternative to the approach you've taken in your RFC patch:
> >
> > arch/m68k/68000/time
Re: [RFC] clang tooling cleanups
On Tue, 27 Oct 2020, t...@redhat.com wrote: > This rfc will describe > An upcoming treewide cleanup. > How clang tooling was used to programatically do the clean up. > Solicit opinions on how to generally use clang tooling. > This tooling is very impressive. It makes possible an idea that I had a while ago, to help make code review more efficient. It works like this. Suppose a patch, p, is the difference between the new tree, n, and the old tree, o. That is, p = n - o. Now let clang-tidy be the transformation 't'. This gets you a much more readable patch submission, P = t(n) - t(o). The only difficulty is that, if I submit P intead of p then 'git am' will probably reject it. This is solved by a little tooling around git, such that, should a patch P fail to apply, the relevant files are automatically reformatted with the officially endorsed transformation t, to generate a minimal cleanup patch, such that P can be automatically applied on top. If the patch submission process required* that every patch submission was generated like P and not like p, it would immediately eliminate all clean-up patches from the workload of all reviewers, and also make the reviewers' job easier because all submissions are now formatted correctly, and also avoid time lost to round-trips, such as, "you can have a reviewed-by if you respin to fix some minor style issues". * Enforcing this, e.g. with checkpatch, is slightly more complicated, but it works the same way: generate a minimal cleanup patch for the relevant files, apply the patch-to-be-submitted, and finally confirm that the modified files are unchanged under t.
Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available
On Thu, 22 Oct 2020, Geert Uytterhoeven wrote: > > Thanks for your patch... > You're welcome. > I can't say I'm a fan of this... > Sorry. > > The real issue is this "extern struct platform_device scc_a_pdev, > scc_b_pdev", circumventing the driver framework. > > Can we get rid of that? > Is there a better alternative? pmz_probe() is called by console_initcall(pmz_console_init) when CONFIG_SERIAL_PMACZILOG_CONSOLE=y because this has to happen earlier than the normal platform bus probing which takes place later as a typical module_initcall.
RE: [PATCH] scsi: megaraid_sas: use spin_lock() in hard IRQ
On Thu, 22 Oct 2020, Tianxianting wrote: > I see, If we add this patch, we need to get all cpu arch that support > nested interrupts. > I was just calling into question 1. the benefit (does it improve performance?) and 2. the code style (is it less portable?). It's really the style question that mostly interests me because I've had to code around the nested interrupt situation before, and everytime it comes up it makes me wonder about the necessity. I was not trying to veto your patch. It is not my position to do that. If Broadcom likes the patch, that's great.
RE: [PATCH] scsi: megaraid_sas: use spin_lock() in hard IRQ
On Thu, 22 Oct 2020, Tianxianting wrote: > Do you mean Megasas raid can be used in m68k arch? m68k is one example of an architecture on which the unstated assumptions in your patch would be invalid. Does this help to clarify what I wrote? If Megasas raid did work on m68k, I'm sure it could potentially benefit from the theoretical performance improvement from your patch. So perhaps you would consider adding support for slower CPUs like m68k.
Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available
On Wed, 21 Oct 2020, Laurent Vivier wrote:
> Le 21/10/2020 à 01:43, Finn Thain a écrit :
>
> > Laurent, can we avoid the irq == 0 warning splat like this?
> >
> > diff --git a/drivers/tty/serial/pmac_zilog.c
> > b/drivers/tty/serial/pmac_zilog.c
> > index 96e7aa479961..7db600cd8cc7 100644
> > --- a/drivers/tty/serial/pmac_zilog.c
> > +++ b/drivers/tty/serial/pmac_zilog.c
> > @@ -1701,8 +1701,10 @@ static int __init pmz_init_port(struct
> > uart_pmac_port *uap)
> > int irq;
> >
> > r_ports = platform_get_resource(uap->pdev, IORESOURCE_MEM, 0);
> > + if (!r_ports)
> > + return -ENODEV;
> > irq = platform_get_irq(uap->pdev, 0);
> > - if (!r_ports || irq <= 0)
> > + if (irq <= 0)
> > return -ENODEV;
> >
> > uap->port.mapbase = r_ports->start;
> >
>
> No, this doesn't fix the problem.
>
Then I had better stop guessing and start up Aranym...
The patch below seems to fix the problem for me. Does it work on your
system(s)?
diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
index a621fcc1a576..4e802f70333d 100644
--- a/arch/m68k/mac/config.c
+++ b/arch/m68k/mac/config.c
@@ -776,16 +776,12 @@ static struct resource scc_b_rsrcs[] = {
struct platform_device scc_a_pdev = {
.name = "scc",
.id = 0,
- .num_resources = ARRAY_SIZE(scc_a_rsrcs),
- .resource = scc_a_rsrcs,
};
EXPORT_SYMBOL(scc_a_pdev);
struct platform_device scc_b_pdev = {
.name = "scc",
.id = 1,
- .num_resources = ARRAY_SIZE(scc_b_rsrcs),
- .resource = scc_b_rsrcs,
};
EXPORT_SYMBOL(scc_b_pdev);
@@ -812,10 +808,15 @@ static void __init mac_identify(void)
/* Set up serial port resources for the console initcall. */
- scc_a_rsrcs[0].start = (resource_size_t) mac_bi_data.sccbase + 2;
- scc_a_rsrcs[0].end = scc_a_rsrcs[0].start;
- scc_b_rsrcs[0].start = (resource_size_t) mac_bi_data.sccbase;
- scc_b_rsrcs[0].end = scc_b_rsrcs[0].start;
+ scc_a_rsrcs[0].start = (resource_size_t)mac_bi_data.sccbase + 2;
+ scc_a_rsrcs[0].end = scc_a_rsrcs[0].start;
+ scc_a_pdev.num_resources = ARRAY_SIZE(scc_a_rsrcs);
+ scc_a_pdev.resource = scc_a_rsrcs;
+
+ scc_b_rsrcs[0].start = (resource_size_t)mac_bi_data.sccbase;
+ scc_b_rsrcs[0].end = scc_b_rsrcs[0].start;
+ scc_b_pdev.num_resources = ARRAY_SIZE(scc_b_rsrcs);
+ scc_b_pdev.resource = scc_b_rsrcs;
switch (macintosh_config->scc_type) {
case MAC_SCC_PSC:
diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
index 96e7aa479961..95abdb305d67 100644
--- a/drivers/tty/serial/pmac_zilog.c
+++ b/drivers/tty/serial/pmac_zilog.c
@@ -1697,18 +1697,17 @@ extern struct platform_device scc_a_pdev, scc_b_pdev;
static int __init pmz_init_port(struct uart_pmac_port *uap)
{
- struct resource *r_ports;
- int irq;
+ struct resource *r_ports, *r_irq;
r_ports = platform_get_resource(uap->pdev, IORESOURCE_MEM, 0);
- irq = platform_get_irq(uap->pdev, 0);
- if (!r_ports || irq <= 0)
+ r_irq = platform_get_resource(uap->pdev, IORESOURCE_IRQ, 0);
+ if (!r_ports || !r_irq)
return -ENODEV;
uap->port.mapbase = r_ports->start;
uap->port.membase = (unsigned char __iomem *) r_ports->start;
uap->port.iotype = UPIO_MEM;
- uap->port.irq = irq;
+ uap->port.irq = r_irq->start;
uap->port.uartclk = ZS_CLOCK;
uap->port.fifosize = 1;
uap->port.ops = _pops;
Re: [PATCH] scsi: megaraid_sas: use spin_lock() in hard IRQ
On Wed, 21 Oct 2020, Xianting Tian wrote:
> Since we already in hard IRQ context when running megasas_isr(),
On m68k, hard irq context does not mean interrupts are disabled. Are there
no other architectures in that category?
> so use spin_lock() is enough, which is faster than spin_lock_irqsave().
>
Is that measurable?
> Signed-off-by: Xianting Tian
> ---
> drivers/scsi/megaraid/megaraid_sas_base.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
> b/drivers/scsi/megaraid/megaraid_sas_base.c
> index 2b7e7b5f3..bd186254d 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -3977,15 +3977,14 @@ static irqreturn_t megasas_isr(int irq, void *devp)
> {
> struct megasas_irq_context *irq_context = devp;
> struct megasas_instance *instance = irq_context->instance;
> - unsigned long flags;
> irqreturn_t rc;
>
> if (atomic_read(>fw_reset_no_pci_access))
> return IRQ_HANDLED;
>
> - spin_lock_irqsave(>hba_lock, flags);
> + spin_lock(>hba_lock);
> rc = megasas_deplete_reply_queue(instance, DID_OK);
> - spin_unlock_irqrestore(>hba_lock, flags);
> + spin_unlock(>hba_lock);
>
> return rc;
> }
>
Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available
On Tue, 20 Oct 2020, Brad Boyer wrote: > > Wouldn't it be better to rearrange this code to only run if the devices > are present? This is a macio driver on pmac and a platform driver on > mac, so shouldn't it be possible to only run this code when the > appropriate entries are present in the right data structures? > > I didn't look at a lot of the other serial drivers, but some other mac > drivers have recently been updated to no longer have MACH_IS_MAC checks > due to being converted to platform drivers. > Actually, it's not simply a platform driver or macio driver. I think the console is supposed to be registered before the normal bus matching takes place. Hence this comment in pmac_zilog.c, /* * First, we need to do a direct OF-based probe pass. We * do that because we want serial console up before the * macio stuffs calls us back, and since that makes it * easier to pass the proper number of channels to * uart_register_driver() */ Laurent, can we avoid the irq == 0 warning splat like this? diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c index 96e7aa479961..7db600cd8cc7 100644 --- a/drivers/tty/serial/pmac_zilog.c +++ b/drivers/tty/serial/pmac_zilog.c @@ -1701,8 +1701,10 @@ static int __init pmz_init_port(struct uart_pmac_port *uap) int irq; r_ports = platform_get_resource(uap->pdev, IORESOURCE_MEM, 0); + if (!r_ports) + return -ENODEV; irq = platform_get_irq(uap->pdev, 0); - if (!r_ports || irq <= 0) + if (irq <= 0) return -ENODEV; uap->port.mapbase = r_ports->start;
Re: [RFC 13/13] m68k: mac: convert to generic clockevent
On Thu, 15 Oct 2020, Arnd Bergmann wrote: > On Thu, Oct 15, 2020 at 3:19 AM Finn Thain wrote: > > > > On Sat, 10 Oct 2020, Arnd Bergmann wrote: > > > > > > Perhaps patch 13 does not belong in this series (?). > > > > > > > > All m68k platforms will need conversion before the TODO can be removed > > > > from Documentation/features/time/clockevents/arch-support.txt. > > > > > > Yes, correct. I marked this patch as RFC instead of PATCH, as I'm just > > > trying to find out where it should be headed. I would hope the other > > > patches can just get merged. > > > > > > > I wonder whether we can improve support for your proposed configuration > > i.e. a system with no oneshot clockevent device. > > > > The 16 platforms you identified are not all in that category but I suspect > > that there are others which are (though they don't appear in this series > > because they already use GENERIC_CLOCKEVENTS). > > > > One useful optimization would be some way to elide oneshot clockevent > > support (perhaps with the help of Link Time Optimization). > > I think this already happens if one picks CONFIG_HZ_PERIODIC while > disabling HIGH_RES_TIMERS. In that case, CONFIG_TICK_ONESHOT > remains disabled. > That configuration still produces the same 5 KiB of bloat. I see that kernel/time/Kconfig has this -- # Core internal switch. Selected by NO_HZ_COMMON / HIGH_RES_TIMERS. This is # only related to the tick functionality. Oneshot clockevent devices # are supported independent of this. config TICK_ONESHOT bool But my question was really about both kinds of dead code (oneshot device support and oneshot tick support). Anyway, after playing with the code for a bit I don't see any easy way to reduce the growth in text size. > ... > > After looking at the chip documentation I don't think it's viable to > > use the hardware timers in the way I proposed. A VIA register access > > requires at least one full VIA clock cycle (about 1.3 us) which means > > register accesses themselves cause timing delays. They also make > > clocksource reads expensive. > > > > I think this rules out oneshot clockevent devices because if the > > system offered such a device it would preferentially get used as a > > tick device. > > > > So I think your approach (periodic clockevent device driven by the > > existing periodic tick interrupt) is best for this platform due to > > simplicity (not much code) and performance (good accuracy, no > > additional overhead). > > Yes, makes sense. I think the one remaining problem with the periodic > mode in this driver is that it can drop timer ticks when interrupts are > disabled for too long, while in oneshot mode there may be a way to know > how much time has passed since the last tick as long as the counter does > not overflow. Is there any benefit from adopting a oneshot tick (rather than periodic) if no clocksource is consulted when calculating the next interval? (I'm assuming NO_HZ is not in use, for reasons discussed below.) > I would agree that despite this oneshot mode is probably worse overall > for timekeeping if the register accesses introduce systematic errors. > It probably has to be tried. But consulting a VIA clocksource on every tick would be expensive on this platform, so if that was the only way to avoid cumulative errors, I'd probably just stick with the periodic tick. > ... > The arm/rpc timer seems to be roughly in the same category as most of > the m68k ones or the i8253 counter on a PC. It's possible that some of > them could use the same logic as drivers/clocksource/i8253.o as long as > there is any hardware oneshot mode. > There appear to be 15 platforms in that category. 4 have no clocksource besides the jiffies clocksource, meaning there's no practical alternative to using a periodic tick, like you did in your RFC patch: arch/m68k/apollo/config.c arch/m68k/q40/q40ints.c arch/m68k/sun3/sun3ints.c arch/m68k/sun3x/time.c The other 11 platforms in that category also have 'synthetic' clocksources derived from a timer reload interrupt. In 3 cases, the clocksource read method does not (or can not) check for a pending counter reload interrupt. For these also, I see no practical alternative to the approach you've taken in your RFC patch: arch/m68k/68000/timers.c arch/m68k/atari/time.c arch/m68k/coldfire/timers.c That leaves 8 platforms that have reliable clocksource devices which should be able to provide an accurate reading even in the presence of a dropped tick (due to drivers disabling interrupts for too long): arch/arm/mach-rpc/time.c arch/m68k/amiga/config.c arch/m68k/bvme6000/config.c arch/m68k/coldfire/sltimers.c arch/m68k/hp300/time.c arch/m68k/mac/via.c arch/m68k/mvme147/config.c arch/m68k/mvme16x/config.c But is there any reason to adopt a oneshot tick on any of these platforms, if NO_HZ won't eliminate the timer interrupt that's needed to run a 'synthetic' clocksource?
Re: [RFC 13/13] m68k: mac: convert to generic clockevent
On Sat, 10 Oct 2020, Arnd Bergmann wrote: > > Perhaps patch 13 does not belong in this series (?). > > > > All m68k platforms will need conversion before the TODO can be removed > > from Documentation/features/time/clockevents/arch-support.txt. > > Yes, correct. I marked this patch as RFC instead of PATCH, as I'm just > trying to find out where it should be headed. I would hope the other > patches can just get merged. > I wonder whether we can improve support for your proposed configuration i.e. a system with no oneshot clockevent device. The 16 platforms you identified are not all in that category but I suspect that there are others which are (though they don't appear in this series because they already use GENERIC_CLOCKEVENTS). One useful optimization would be some way to elide oneshot clockevent support (perhaps with the help of Link Time Optimization). > > On m68k, HZ is fixed at 100. Without addressing that, would there be > > any benefit from adopting GENERIC_CLOCKEVENTS as per this RFC patch? > > I don't think so, I mainly did it to see if there is a problem with > mixing the two modes, and I couldn't find any. The behavior seems > unchanged before and after my patch, the main difference being a few > extra kilobytes in kernel .text for the generic clockevents code. > I think that is a good reason to convert all m68k platforms at once and to elide some of the dead code. > > On Thu, 8 Oct 2020, Arnd Bergmann wrote: > > > > > Now that the infrastructure allows kernels to have both legacy timer > > > ticks and clockevent drivers in the same image, start by moving one > > > platform to generic clockevents. > > > > > > As qemu only supports the q800 platform among the classic m68k, use > > > that as an example. > > > > > > > Correct VIA emulation is suprisingly difficult, so this kind of work > > should be tested on real hardware. > > > > I say that because when I did the clocksource conversion for m68k I > > ran into a bug in QEMU (since fixed) and also because I once worked on > > some of the bugs in the emulated VIA device used in MAME/MESS. > > Good point, though I would be surprised if anything went wrong with this > patch on real hardware but not in emulation, as all the register-level > interactions with the timer are the same. > On the subject of register accesses, via1[ACR] is shared with ADB drivers, so this patch probably has to protect those accesses with local_irq_save/restore or local_irq_disable/enable. (I can't be sure of the contexts in which .set_state_shutdown and .set_state_periodic methods are called.) > Adding oneshot mode is a completely different matter though, that > clearly needs to be tested on real hardware. > Right, and many emulators trade-off timing accuracy for performance which makes them unsuitable for testing invasive changes of that sort. > > > I also tried adding oneshot mode, which was successful but broke the > > > clocksource. It's probably not hard to make it work properly, but > > > this is where I've stopped. > > > > > > > I'm not so sure that one timer is able to support both a clocksource > > driver and a clockevent driver. In some cases we may have to drop the > > clocksource driver (i.e. fall back on the jiffies clocksource). > > > > Anyway, even on Macs with only one VIA chip we still have two timers. > > So I think we should try to use Timer 1 as a freerunning clocksource > > and Timer 2 as a oneshot clock event. This may result in better > > accuracy and simpler code. This may require some experimentation > > though. > > Ah, good. This is partly what I had been hoping for, as my patch can be > used as a starting point for that if you want to give it a go. > After looking at the chip documentation I don't think it's viable to use the hardware timers in the way I proposed. A VIA register access requires at least one full VIA clock cycle (about 1.3 us) which means register accesses themselves cause timing delays. They also make clocksource reads expensive. I think this rules out oneshot clockevent devices because if the system offered such a device it would preferentially get used as a tick device. So I think your approach (periodic clockevent device driven by the existing periodic tick interrupt) is best for this platform due to simplicity (not much code) and performance (good accuracy, no additional overhead). I suspect the same approach would work equally well on other platforms too (even though they are probably be capable of oneshot clockevent devices). > Arnd >
Re: [PATCH v3] qla2xxx: Return EBUSY on fcport deletion
On Tue, 13 Oct 2020, Daniel Wagner wrote:
> On Tue, Oct 13, 2020 at 10:59:18AM +1100, Finn Thain wrote:
> >
> > On Mon, 12 Oct 2020, Daniel Wagner wrote:
> >
> > > When the fcport is about to be deleted we should return EBUSY
> > > instead of ENODEV. Only for EBUSY the request will be requeued in a
> > > multipath setup.
> > >
> > > Also in case we have a valid qpair but the firmware has not yet
> > > started return EBUSY to avoid dropping the request.
> > >
> > > Signed-off-by: Daniel Wagner
> > > ---
> > >
> > > v3: simplify test logic as suggested by Arun.
> >
> > Not exactly a "simplification": there was a change of behaviour
> > between v2 and v3. It seems the commit log no longer reflects the
> > code.
>
> How so? I am struggling to see how it could be a change in behavior. But
> then I sometimes fail at simple logic ;)
>
Me too, so I confirmed the result by executing the code snippets.
> v2 and v3 will return ENODEV if qpair or fcport are invalid and for
> EBUSY one of the other condition needs be true. The difference between
> v2 and v3 should only be the order how tests are executed. The outcome
> should be the same.
>
Here's a truth table for v2:
qpair fw_started fcport deleted result
---
F X F X -ENODEV
F F T F -ENODEV
F F T T -EBUSY *
F T T F -ENODEV
F T T T -EBUSY *
T F F X -EBUSY *
T F T X -EBUSY
T T F X -ENODEV
T T T F neither
T T T T -EBUSY
Here's a truth table for v3:
qpair fw_started fcport deleted result
---
F X F X -ENODEV
F F T F -ENODEV
F F T T -ENODEV *
F T T F -ENODEV
F T T T -ENODEV *
T F F X -ENODEV *
T F T X -EBUSY
T T F X -ENODEV
T T T F neither
T T T T -EBUSY
The asterisks mark the changed rows.
I don't know whether the changes in v3 are desirable or not, I was just
pointing out that the commit log ("valid qpair but the firmware has not
yet started return EBUSY") now seems to disagree with the code.
Re: [PATCH v3] qla2xxx: Return EBUSY on fcport deletion
On Mon, 12 Oct 2020, Daniel Wagner wrote: > When the fcport is about to be deleted we should return EBUSY instead > of ENODEV. Only for EBUSY the request will be requeued in a multipath > setup. > > Also in case we have a valid qpair but the firmware has not yet > started return EBUSY to avoid dropping the request. > > Signed-off-by: Daniel Wagner > --- > > v3: simplify test logic as suggested by Arun. Not exactly a "simplification": there was a change of behaviour between v2 and v3. It seems the commit log no longer reflects the code. > v2: rebased on mkp/staging > > drivers/scsi/qla2xxx/qla_nvme.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c > index 2cd9bd288910..1fa457a5736e 100644 > --- a/drivers/scsi/qla2xxx/qla_nvme.c > +++ b/drivers/scsi/qla2xxx/qla_nvme.c > @@ -555,10 +555,12 @@ static int qla_nvme_post_cmd(struct nvme_fc_local_port > *lport, > > fcport = qla_rport->fcport; > > - if (!qpair || !fcport || (qpair && !qpair->fw_started) || > - (fcport && fcport->deleted)) > + if (!qpair || !fcport) > return -ENODEV; > > + if (!qpair->fw_started || fcport->deleted) > + return -EBUSY; > + > vha = fcport->vha; > > if (!(fcport->nvme_flag & NVME_FLAG_REGISTERED)) >
Re: [RFC 13/13] m68k: mac: convert to generic clockevent
Hi Arnd, Perhaps patch 13 does not belong in this series (?). All m68k platforms will need conversion before the TODO can be removed from Documentation/features/time/clockevents/arch-support.txt. On m68k, HZ is fixed at 100. Without addressing that, would there be any benefit from adopting GENERIC_CLOCKEVENTS as per this RFC patch? On Thu, 8 Oct 2020, Arnd Bergmann wrote: > Now that the infrastructure allows kernels to have both legacy timer > ticks and clockevent drivers in the same image, start by moving one > platform to generic clockevents. > > As qemu only supports the q800 platform among the classic m68k, use that > as an example. > Correct VIA emulation is suprisingly difficult, so this kind of work should be tested on real hardware. I say that because when I did the clocksource conversion for m68k I ran into a bug in QEMU (since fixed) and also because I once worked on some of the bugs in the emulated VIA device used in MAME/MESS. > I also tried adding oneshot mode, which was successful but broke the > clocksource. It's probably not hard to make it work properly, but this > is where I've stopped. > I'm not so sure that one timer is able to support both a clocksource driver and a clockevent driver. In some cases we may have to drop the clocksource driver (i.e. fall back on the jiffies clocksource). Anyway, even on Macs with only one VIA chip we still have two timers. So I think we should try to use Timer 1 as a freerunning clocksource and Timer 2 as a oneshot clock event. This may result in better accuracy and simpler code. This may require some experimentation though. > Signed-off-by: Arnd Bergmann > --- > I have never tried implementing a clockevent or clocksource > driver in the past, so this is really just an experiment and > I expect I got something wrong. > Writing clockevent drivers is new to me too. I'm still trying to discover what the implications might be if the only available clockevent device offers oneshot mode or periodic mode but not both.
Re: [PATCH 01/13] timekeeping: add CONFIG_LEGACY_TIMER_TICK
Hi Arnd,
On Thu, 8 Oct 2020, Arnd Bergmann wrote:
> All platforms that currently do not use generic clockevents roughly call
> the same set of functions in their timer interrupts: xtime_update(),
> update_process_times() and profile_tick(), sometimes in a different
> sequence.
>
> Add a helper function that performs all three of them, to make the
> callers more uniform and simplify the interface.
>
> Signed-off-by: Arnd Bergmann
> ---
> include/linux/timekeeping.h | 1 +
> kernel/time/Kconfig | 7 +++
> kernel/time/Makefile| 1 +
> kernel/time/tick-legacy.c | 19 +++
> 4 files changed, 28 insertions(+)
> create mode 100644 kernel/time/tick-legacy.c
>
> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> index 7f7e4a3f4394..3670cb1670ff 100644
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -12,6 +12,7 @@ extern int timekeeping_suspended;
> /* Architecture timer tick functions: */
> extern void update_process_times(int user);
> extern void xtime_update(unsigned long ticks);
> +extern void legacy_timer_tick(unsigned long ticks);
>
> /*
> * Get and set timeofday
> diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig
> index a09b1d61df6a..f2b0cfeade47 100644
> --- a/kernel/time/Kconfig
> +++ b/kernel/time/Kconfig
> @@ -61,6 +61,13 @@ config POSIX_CPU_TIMERS_TASK_WORK
> bool
> default y if POSIX_TIMERS && HAVE_POSIX_CPU_TIMERS_TASK_WORK
>
> +config LEGACY_TIMER_TICK
> + bool
> + help
> + The legacy timer tick helper is used by platforms that
> + lack support for the generic clockevent framework.
> + New platforms should use generic clockevents instead.
> +
> if GENERIC_CLOCKEVENTS
> menu "Timers subsystem"
>
> diff --git a/kernel/time/Makefile b/kernel/time/Makefile
> index c8f00168afe8..1fb1c1ef6a19 100644
> --- a/kernel/time/Makefile
> +++ b/kernel/time/Makefile
> @@ -16,6 +16,7 @@ ifeq ($(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST),y)
> endif
> obj-$(CONFIG_GENERIC_SCHED_CLOCK)+= sched_clock.o
> obj-$(CONFIG_TICK_ONESHOT) += tick-oneshot.o tick-sched.o
> +obj-$(CONFIG_LEGACY_TIMER_TICK) += tick-legacy.o
> obj-$(CONFIG_HAVE_GENERIC_VDSO) += vsyscall.o
> obj-$(CONFIG_DEBUG_FS) += timekeeping_debug.o
> obj-$(CONFIG_TEST_UDELAY)+= test_udelay.o
> diff --git a/kernel/time/tick-legacy.c b/kernel/time/tick-legacy.c
> new file mode 100644
> index ..73c5a0af4743
> --- /dev/null
> +++ b/kernel/time/tick-legacy.c
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Timer tick function for architectures that lack generic clockevents,
> + * consolidated here from m68k/ia64/parisc/arm.
> + */
> +
> +#include
> +#include
> +#include
> +
> +#include "tick-internal.h"
> +
> +void legacy_timer_tick(unsigned long ticks)
> +{
> + if (ticks)
> + xtime_update(ticks);
> + update_process_times(user_mode(get_irq_regs()));
> + profile_tick(CPU_PROFILING);
> +}
>
It's good to see this code refactored in this way because, as well as
de-duplication, it reveals the logic that's common to the relevant
platforms and may shed some light on the need for that logic.
Yet it's not clear to me that the clockevents framework is able to replace
that logic on all of the affected hardware. I suppose it remains to be
seen.
I hate to quibble about naming, but you seem to be using "legacy" here to
mean "deprecated" (?) Is it a good idea to prepend such adjectives to
symbol names?
IMO, the term "legacy" is redundant in this context. That term covers a
large portion of kernel code, a large number of hardware features in
current silicon, a large portion of the userspace ABI, a large number of
production Linux systems, probably all "Unix" systems, etc.
As a corollary, cutting edge ("non-legacy") code is often kept out of open
source projects by the owners of the intellectual property rights.
Re: [PATCH] m68k: remove unused mach_max_dma_address
On Fri, 9 Oct 2020, Laurent Vivier wrote:
> This information is unused since the discontinuous memory support
> has been introduced in 2007.
>
> Fixes: 12d810c1b8c2 ("m68k: discontinuous memory support")
Does this qualify as a bug fix?
Re: [v5 01/12] struct device: Add function callback durable_name
On Wed, 7 Oct 2020, Tony Asleson wrote:
> The log information is not helpful without the information to correlate
> to the actual device.
Log messages that associate one entity with another can be generated
whenever such an association comes into existence, which is probably when
devices get probed.
E.g. a host:channel:target:lun identifier gets associated with a block
device name by the dev_printk() calls in sd_probe():
[3.60] sd 0:0:0:0: [sda] Attached SCSI disk
BTW, if you think of {"0:0:0:0","sda"} as a row in some normalized table
and squint a bit, this problem is not unlike the replication of database
tables over a message queue.
[PATCH] ide/falconide: Fix module unload
Unloading the falconide module results in a crash:
Unable to handle kernel NULL pointer dereference at virtual address
Oops:
Modules linked in: falconide(-)
PC: [<002930b2>] ide_host_remove+0x2e/0x1d2
SR: 2000 SP: 00b49e28 a2: 009b0f90
d0: d1: 009b0f90d2: d3: 00b48000
d4: 003cef32d5: 00299188a0: 0086d000a1: 0086d000
Process rmmod (pid: 322, task=009b0f90)
Frame format=7 eff addr= ssw=0505 faddr=
wb 1 stat/addr/data:
wb 2 stat/addr/data:
wb 3 stat/addr/data: 00018da9
push data:
Stack from 00b49e90:
004c456a 0027f176 0027cb0a 0027cb9e 0086d00a 2187d3f0 0027f0e0
00b49ebc 2187d1f6 00b49ec8 002811e8 0086d000 00b49ef0 0028024c
0086d00a 002800d6 00279a1a 0001 0001 0086d00a 2187d3f0 00279a58
00b49f1c 002802e0 0086d00a 2187d3f0 004c456a 0086d00a ef96af74
2187d3f0 002805d2 800de064 00b49f44 0027f088 2187d3f0 00ac1cf4 2187d3f0
004c43be 2187d3f0 2187d3f0 800b66a8 00b49f5c 00280776 2187d3f0
Call Trace: [<0027f176>] __device_driver_unlock+0x0/0x48
[<0027cb0a>] device_links_busy+0x0/0x94
[<0027cb9e>] device_links_unbind_consumers+0x0/0x130
[<0027f0e0>] __device_driver_lock+0x0/0x5a
[<2187d1f6>] falconide_remove+0x12/0x18 [falconide]
[<002811e8>] platform_drv_remove+0x1c/0x28
[<0028024c>] device_release_driver_internal+0x176/0x17c
[<002800d6>] device_release_driver_internal+0x0/0x17c
[<00279a1a>] get_device+0x0/0x22
[<00279a58>] put_device+0x0/0x18
[<002802e0>] driver_detach+0x56/0x82
[<002805d2>] driver_remove_file+0x0/0x24
[<0027f088>] bus_remove_driver+0x4c/0xa4
[<00280776>] driver_unregister+0x28/0x5a
[<00281a00>] platform_driver_unregister+0x12/0x18
[<2187d2a0>] ide_falcon_driver_exit+0x10/0x16 [falconide]
[<000764f0>] sys_delete_module+0x110/0x1f2
[<000e83ea>] sys_rename+0x1a/0x1e
[<2e0c>] syscall+0x8/0xc
[<00188004>] ext4_multi_mount_protect+0x35a/0x3ce
Code: 0029 9188 4bf9 0027 aa1c 283c 003c ef32 <265c> 4a8b 6700 00b8 2043 2028
000c 0280 00ff ff00 6600 0176 40c0 7202 b2b9 004c
Disabling lock debugging due to kernel taint
This happens because the driver_data pointer is uninitialized.
Add the missing platform_set_drvdata() call. For clarity, use the
matching platform_get_drvdata() as well.
Cc: Michael Schmitz
Cc: Bartlomiej Zolnierkiewicz
Cc: linux-m...@lists.linux-m68k.org
Fixes: 5ed0794cde593 ("m68k/atari: Convert Falcon IDE drivers to platform
drivers")
Signed-off-by: Finn Thain
---
This patch was tested using Aranym.
---
drivers/ide/falconide.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/ide/falconide.c b/drivers/ide/falconide.c
index dbeb2605e5f6e..607c44bc50f1b 100644
--- a/drivers/ide/falconide.c
+++ b/drivers/ide/falconide.c
@@ -166,6 +166,7 @@ static int __init falconide_init(struct platform_device
*pdev)
if (rc)
goto err_free;
+ platform_set_drvdata(pdev, host);
return 0;
err_free:
ide_host_free(host);
@@ -176,7 +177,7 @@ static int __init falconide_init(struct platform_device
*pdev)
static int falconide_remove(struct platform_device *pdev)
{
- struct ide_host *host = dev_get_drvdata(>dev);
+ struct ide_host *host = platform_get_drvdata(pdev);
ide_host_remove(host);
--
2.26.2
Re: [PATCH v2] ide/macide: Convert Mac IDE driver to platform driver
Hi Geert,
On Wed, 16 Sep 2020, Finn Thain wrote:
> On Tue, 15 Sep 2020, Geert Uytterhoeven wrote:
>
> > > > > --- a/drivers/ide/macide.c
> > > > > +++ b/drivers/ide/macide.c
> > > >
> > > > > @@ -109,42 +110,61 @@ static const char *mac_ide_name[] =
> > > > > * Probe for a Macintosh IDE interface
> > > > > */
> > > > >
> > > > > -static int __init macide_init(void)
> > > > > +static int mac_ide_probe(struct platform_device *pdev)
> > > > > {
> > > >
> > > > > printk(KERN_INFO "ide: Macintosh %s IDE controller\n",
> > > > > mac_ide_name[macintosh_config->ide_type -
> > > > > 1]);
> > > > >
> > > > > - macide_setup_ports(, base, irq);
> > > > > + macide_setup_ports(, mem->start, irq->start);
> > > > >
> > > > > - return ide_host_add(, hws, 1, NULL);
> > > > > + rc = ide_host_add(, hws, 1, );
> > > > > + if (rc)
> > > > > + return rc;
> > > > > +
> > > > > + platform_set_drvdata(pdev, host);
> > > >
> > > > Move one up, to play it safe?
> > > >
> > >
> > > You mean, before calling ide_host_add? The 'host' pointer is
> > > uninitialized prior to that call.
> >
> > Oh right, so the IDE subsystem doesn't let you use the drvdata inside
> > your driver (besides in remove()) in a safe way :-(
> >
>
> The IDE subsystem does allow other patterns here. I could have changed
> ide_host_alloc() into ide_host_register() followed by ide_host_add() but
> I could not see any benefit from that change.
>
Sorry, I meant to say, "I could have changed ide_host_add() into
ide_host_alloc() followed by ide_host_register() ..."
> A quick search for "platform_device" shows that the driver does not use
> any uninitialized driver_data pointer (because ide_ifr is a global). In
> your message of September 9th you readily reached the same conclusion
> when you reviewed v1.
>
> If mac_ide_probe() followed the usual pattern it might make review
> easier (as reviewers may not wish to consider the entire driver) but
> does that really make the code more "safe"?
>
I still think that "if it ain't broke, don't fix it" is actually the
"safe" option for macide.c. But I'm happy to make additional changes, test
them and send v5 if that's preferred.
Looking further at the drivers using ide_host_register(), I see that
falconide.c is missing a set_drvdata() call, while tx4939ide.c calls
set_drvdata() after ide_host_register(). The latter example is not a bug.
The pattern I used, that is, calling set_drvdata() after ide_host_add(),
is actually more popular among IDE drivers than the pattern you suggested,
that is, set_drvdata() followed by ide_host_register(). Either way, I
don't see any bugs besides the one in falconide.c.
Regarding falconide.c, my inclination is to send a fix following the more
common pattern (among IDE drivers), as below. I guess that may prompt the
subsystem maintainers to make known their views on the style question.
diff --git a/drivers/ide/falconide.c b/drivers/ide/falconide.c
index dbeb2605e5f6e..607c44bc50f1b 100644
--- a/drivers/ide/falconide.c
+++ b/drivers/ide/falconide.c
@@ -166,6 +166,7 @@ static int __init falconide_init(struct platform_device
*pdev)
if (rc)
goto err_free;
+ platform_set_drvdata(pdev, host);
return 0;
err_free:
ide_host_free(host);
@@ -176,7 +177,7 @@ static int __init falconide_init(struct platform_device
*pdev)
static int falconide_remove(struct platform_device *pdev)
{
- struct ide_host *host = dev_get_drvdata(>dev);
+ struct ide_host *host = platform_get_drvdata(pdev);
ide_host_remove(host);
[PATCH v4] ide/macide: Convert Mac IDE driver to platform driver
Add platform devices for the Mac IDE controller variants. Convert the
macide module into a platform driver to support two of those variants.
For the third, use a generic "pata_platform" driver instead.
This enables automatic loading of the appropriate module and begins
the process of replacing the driver with libata alternatives.
Cc: Bartlomiej Zolnierkiewicz
Cc: Geert Uytterhoeven
Cc: Joshua Thompson
References: commit 5ed0794cde593 ("m68k/atari: Convert Falcon IDE drivers to
platform drivers")
References: commit 7ad19a99ad431 ("ide: officially deprecated the legacy IDE
driver")
Tested-by: Stan Johnson
Signed-off-by: Finn Thain
---
This patch was tested successfully on a Powerbook 190 (MAC_IDE_BABOON)
using both pata_platform and ide_platform drivers.
The next step will be to try using these generic drivers with the other
IDE controller variants (MAC_IDE_QUADRA or MAC_IDE_PB) so that the macide
driver can be entirely replaced with libata drivers.
Changed since v3:
- Updated Kconfig help text.
Changed since v2:
- Enabled CONFIG_BLK_DEV_PLATFORM in multi_defconfig.
- Replaced dev_get_drvdata() with platform_get_drvdata().
Changed since v1:
- Adopted DEFINE_RES_MEM and DEFINE_RES_IRQ macros.
- Dropped IORESOURCE_IRQ_SHAREABLE flag as it is ignored by pata_platform.c
and IRQF_SHARED makes no difference in this case.
- Removed redundant release_mem_region() call.
- Enabled CONFIG_BLK_DEV_PLATFORM in mac_defconfig. We might also enable
CONFIG_PATA_PLATFORM but IMO migration to libata should be a separate
patch (as this patch has some unrelated benefits).
---
arch/m68k/configs/mac_defconfig | 1 +
arch/m68k/configs/multi_defconfig | 1 +
arch/m68k/mac/config.c| 41 +++
drivers/ide/Kconfig | 7 ++--
drivers/ide/macide.c | 66 ---
5 files changed, 90 insertions(+), 26 deletions(-)
diff --git a/arch/m68k/configs/mac_defconfig b/arch/m68k/configs/mac_defconfig
index 6087798662601..f770970fe4e99 100644
--- a/arch/m68k/configs/mac_defconfig
+++ b/arch/m68k/configs/mac_defconfig
@@ -317,6 +317,7 @@ CONFIG_DUMMY_IRQ=m
CONFIG_IDE=y
CONFIG_IDE_GD_ATAPI=y
CONFIG_BLK_DEV_IDECD=y
+CONFIG_BLK_DEV_PLATFORM=y
CONFIG_BLK_DEV_MAC_IDE=y
CONFIG_RAID_ATTRS=m
CONFIG_SCSI=y
diff --git a/arch/m68k/configs/multi_defconfig
b/arch/m68k/configs/multi_defconfig
index 0abb53c38c20d..f93c3021f20d4 100644
--- a/arch/m68k/configs/multi_defconfig
+++ b/arch/m68k/configs/multi_defconfig
@@ -346,6 +346,7 @@ CONFIG_DUMMY_IRQ=m
CONFIG_IDE=y
CONFIG_IDE_GD_ATAPI=y
CONFIG_BLK_DEV_IDECD=y
+CONFIG_BLK_DEV_PLATFORM=y
CONFIG_BLK_DEV_GAYLE=y
CONFIG_BLK_DEV_BUDDHA=y
CONFIG_BLK_DEV_FALCON_IDE=y
diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
index 5c9f3a2d65388..43fc29180cb58 100644
--- a/arch/m68k/mac/config.c
+++ b/arch/m68k/mac/config.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -940,6 +941,26 @@ static const struct resource mac_scsi_ccl_rsrc[]
__initconst = {
},
};
+static const struct resource mac_ide_quadra_rsrc[] __initconst = {
+ DEFINE_RES_MEM(0x50F1A000, 0x104),
+ DEFINE_RES_IRQ(IRQ_NUBUS_F),
+};
+
+static const struct resource mac_ide_pb_rsrc[] __initconst = {
+ DEFINE_RES_MEM(0x50F1A000, 0x104),
+ DEFINE_RES_IRQ(IRQ_NUBUS_C),
+};
+
+static const struct resource mac_pata_baboon_rsrc[] __initconst = {
+ DEFINE_RES_MEM(0x50F1A000, 0x38),
+ DEFINE_RES_MEM(0x50F1A038, 0x04),
+ DEFINE_RES_IRQ(IRQ_BABOON_1),
+};
+
+static const struct pata_platform_info mac_pata_baboon_data __initconst = {
+ .ioport_shift = 2,
+};
+
int __init mac_platform_init(void)
{
phys_addr_t swim_base = 0;
@@ -1048,6 +1069,26 @@ int __init mac_platform_init(void)
break;
}
+ /*
+* IDE device
+*/
+
+ switch (macintosh_config->ide_type) {
+ case MAC_IDE_QUADRA:
+ platform_device_register_simple("mac_ide", -1,
+ mac_ide_quadra_rsrc, ARRAY_SIZE(mac_ide_quadra_rsrc));
+ break;
+ case MAC_IDE_PB:
+ platform_device_register_simple("mac_ide", -1,
+ mac_ide_pb_rsrc, ARRAY_SIZE(mac_ide_pb_rsrc));
+ break;
+ case MAC_IDE_BABOON:
+ platform_device_register_resndata(NULL, "pata_platform", -1,
+ mac_pata_baboon_rsrc, ARRAY_SIZE(mac_pata_baboon_rsrc),
+ _pata_baboon_data, sizeof(mac_pata_baboon_data));
+ break;
+ }
+
/*
* Ethernet device
*/
diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
index 973ed4b684cec..19abf11c84c8a 100644
--- a/drivers/ide/Kconfig
+++ b/drivers/ide/Kconfig
@@ -744,9 +744,10 @@ config BLK_DEV_MAC_IDE
depends on MAC
help
This is the IDE driver
Re: [PATCH 1/9] kernel: add a PF_FORCE_COMPAT flag
On Sat, 19 Sep 2020, Arnd Bergmann wrote: > On Sat, Sep 19, 2020 at 6:21 PM Andy Lutomirski wrote: > > On Fri, Sep 18, 2020 at 8:16 AM Christoph Hellwig wrote: > > > On Fri, Sep 18, 2020 at 02:58:22PM +0100, Al Viro wrote: > > > > Said that, why not provide a variant that would take an explicit > > > > "is it compat" argument and use it there? And have the normal one > > > > pass in_compat_syscall() to that... > > > > > > That would help to not introduce a regression with this series yes. > > > But it wouldn't fix existing bugs when io_uring is used to access > > > read or write methods that use in_compat_syscall(). One example > > > that I recently ran into is drivers/scsi/sg.c. > > Ah, so reading /dev/input/event* would suffer from the same issue, and > that one would in fact be broken by your patch in the hypothetical case > that someone tried to use io_uring to read /dev/input/event on x32... > > For reference, I checked the socket timestamp handling that has a number > of corner cases with time32/time64 formats in compat mode, but none of > those appear to be affected by the problem. > > > Aside from the potentially nasty use of per-task variables, one thing > > I don't like about PF_FORCE_COMPAT is that it's one-way. If we're > > going to have a generic mechanism for this, shouldn't we allow a full > > override of the syscall arch instead of just allowing forcing compat > > so that a compat syscall can do a non-compat operation? > > The only reason it's needed here is that the caller is in a kernel > thread rather than a system call. Are there any possible scenarios where > one would actually need the opposite? > Quite possibly. The ext4 vs. compat getdents bug is still unresolved. Please see, https://lore.kernel.org/lkml/cafeaca9w+jk7_trttnl1p2es1knnpjx9wcuvhflwxlq9aug...@mail.gmail.com/ >Arnd >
Re: [PATCH] scsi: remove redundant initialization of variable ret
On Fri, 18 Sep 2020, Jing Xiangfeng wrote: > The variable ret is being initialized with '-ENOMEM' that is > meaningless. So remove it. > Acked-by: Finn Thain
Re: [PATCH v2] ide/macide: Convert Mac IDE driver to platform driver
On Tue, 15 Sep 2020, Geert Uytterhoeven wrote:
> > > > --- a/drivers/ide/macide.c
> > > > +++ b/drivers/ide/macide.c
> > >
> > > > @@ -109,42 +110,61 @@ static const char *mac_ide_name[] =
> > > > * Probe for a Macintosh IDE interface
> > > > */
> > > >
> > > > -static int __init macide_init(void)
> > > > +static int mac_ide_probe(struct platform_device *pdev)
> > > > {
> > >
> > > > printk(KERN_INFO "ide: Macintosh %s IDE controller\n",
> > > > mac_ide_name[macintosh_config->ide_type - 1]);
> > > >
> > > > - macide_setup_ports(, base, irq);
> > > > + macide_setup_ports(, mem->start, irq->start);
> > > >
> > > > - return ide_host_add(, hws, 1, NULL);
> > > > + rc = ide_host_add(, hws, 1, );
> > > > + if (rc)
> > > > + return rc;
> > > > +
> > > > + platform_set_drvdata(pdev, host);
> > >
> > > Move one up, to play it safe?
> > >
> >
> > You mean, before calling ide_host_add? The 'host' pointer is uninitialized
> > prior to that call.
>
> Oh right, so the IDE subsystem doesn't let you use the drvdata inside
> your driver (besides in remove()) in a safe way :-(
>
The IDE subsystem does allow other patterns here. I could have changed
ide_host_alloc() into ide_host_register() followed by ide_host_add() but I
could not see any benefit from that change.
A quick search for "platform_device" shows that the driver does not use
any uninitialized driver_data pointer (because ide_ifr is a global). In
your message of September 9th you readily reached the same conclusion when
you reviewed v1.
If mac_ide_probe() followed the usual pattern it might make review easier
(as reviewers may not wish to consider the entire driver) but does that
really make the code more "safe"?
[PATCH v3] ide/macide: Convert Mac IDE driver to platform driver
Add platform devices for the Mac IDE controller variants. Convert the
macide module into a platform driver to support two of those variants.
For the third, use a generic "pata_platform" driver instead.
This enables automatic loading of the appropriate module and begins
the process of replacing the driver with libata alternatives.
Cc: Bartlomiej Zolnierkiewicz
Cc: Geert Uytterhoeven
Cc: Joshua Thompson
References: commit 5ed0794cde593 ("m68k/atari: Convert Falcon IDE drivers to
platform drivers")
References: commit 7ad19a99ad431 ("ide: officially deprecated the legacy IDE
driver")
Tested-by: Stan Johnson
Signed-off-by: Finn Thain
---
This patch was tested successfully on a Powerbook 190 (MAC_IDE_BABOON)
using both pata_platform and ide_platform drivers.
The next step will be to try using these generic drivers with the other
IDE controller variants (MAC_IDE_QUADRA or MAC_IDE_PB) so that the macide
driver can be entirely replaced with libata drivers.
Changed since v2:
- Enabled CONFIG_BLK_DEV_PLATFORM in multi_defconfig.
- Replaced dev_get_drvdata() with platform_get_drvdata().
Changed since v1:
- Adopted DEFINE_RES_MEM and DEFINE_RES_IRQ macros.
- Dropped IORESOURCE_IRQ_SHAREABLE flag as it is ignored by pata_platform.c
and IRQF_SHARED makes no difference in this case.
- Removed redundant release_mem_region() call.
- Enabled CONFIG_BLK_DEV_PLATFORM in mac_defconfig. We might also enable
CONFIG_PATA_PLATFORM but IMO migration to libata should be a separate
patch (as this patch has some unrelated benefits).
---
arch/m68k/configs/mac_defconfig | 1 +
arch/m68k/configs/multi_defconfig | 1 +
arch/m68k/mac/config.c| 41 +++
drivers/ide/macide.c | 66 ---
4 files changed, 86 insertions(+), 23 deletions(-)
diff --git a/arch/m68k/configs/mac_defconfig b/arch/m68k/configs/mac_defconfig
index 6087798662601..f770970fe4e99 100644
--- a/arch/m68k/configs/mac_defconfig
+++ b/arch/m68k/configs/mac_defconfig
@@ -317,6 +317,7 @@ CONFIG_DUMMY_IRQ=m
CONFIG_IDE=y
CONFIG_IDE_GD_ATAPI=y
CONFIG_BLK_DEV_IDECD=y
+CONFIG_BLK_DEV_PLATFORM=y
CONFIG_BLK_DEV_MAC_IDE=y
CONFIG_RAID_ATTRS=m
CONFIG_SCSI=y
diff --git a/arch/m68k/configs/multi_defconfig
b/arch/m68k/configs/multi_defconfig
index 0abb53c38c20d..f93c3021f20d4 100644
--- a/arch/m68k/configs/multi_defconfig
+++ b/arch/m68k/configs/multi_defconfig
@@ -346,6 +346,7 @@ CONFIG_DUMMY_IRQ=m
CONFIG_IDE=y
CONFIG_IDE_GD_ATAPI=y
CONFIG_BLK_DEV_IDECD=y
+CONFIG_BLK_DEV_PLATFORM=y
CONFIG_BLK_DEV_GAYLE=y
CONFIG_BLK_DEV_BUDDHA=y
CONFIG_BLK_DEV_FALCON_IDE=y
diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
index 5c9f3a2d65388..43fc29180cb58 100644
--- a/arch/m68k/mac/config.c
+++ b/arch/m68k/mac/config.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -940,6 +941,26 @@ static const struct resource mac_scsi_ccl_rsrc[]
__initconst = {
},
};
+static const struct resource mac_ide_quadra_rsrc[] __initconst = {
+ DEFINE_RES_MEM(0x50F1A000, 0x104),
+ DEFINE_RES_IRQ(IRQ_NUBUS_F),
+};
+
+static const struct resource mac_ide_pb_rsrc[] __initconst = {
+ DEFINE_RES_MEM(0x50F1A000, 0x104),
+ DEFINE_RES_IRQ(IRQ_NUBUS_C),
+};
+
+static const struct resource mac_pata_baboon_rsrc[] __initconst = {
+ DEFINE_RES_MEM(0x50F1A000, 0x38),
+ DEFINE_RES_MEM(0x50F1A038, 0x04),
+ DEFINE_RES_IRQ(IRQ_BABOON_1),
+};
+
+static const struct pata_platform_info mac_pata_baboon_data __initconst = {
+ .ioport_shift = 2,
+};
+
int __init mac_platform_init(void)
{
phys_addr_t swim_base = 0;
@@ -1048,6 +1069,26 @@ int __init mac_platform_init(void)
break;
}
+ /*
+* IDE device
+*/
+
+ switch (macintosh_config->ide_type) {
+ case MAC_IDE_QUADRA:
+ platform_device_register_simple("mac_ide", -1,
+ mac_ide_quadra_rsrc, ARRAY_SIZE(mac_ide_quadra_rsrc));
+ break;
+ case MAC_IDE_PB:
+ platform_device_register_simple("mac_ide", -1,
+ mac_ide_pb_rsrc, ARRAY_SIZE(mac_ide_pb_rsrc));
+ break;
+ case MAC_IDE_BABOON:
+ platform_device_register_resndata(NULL, "pata_platform", -1,
+ mac_pata_baboon_rsrc, ARRAY_SIZE(mac_pata_baboon_rsrc),
+ _pata_baboon_data, sizeof(mac_pata_baboon_data));
+ break;
+ }
+
/*
* Ethernet device
*/
diff --git a/drivers/ide/macide.c b/drivers/ide/macide.c
index 3c6bb8599303b..8a201a467886b 100644
--- a/drivers/ide/macide.c
+++ b/drivers/ide/macide.c
@@ -18,10 +18,11 @@
#include
#include
#include
+#include
#include
-#include
-#include
+
+#define DRV_NAME "mac_ide"
#define IDE_BASE 0x50F1A000/* Base address of IDE controller
Re: [PATCH v2] ide/macide: Convert Mac IDE driver to platform driver
On Mon, 14 Sep 2020, Geert Uytterhoeven wrote:
> Hi Finn,
>
> On Mon, Sep 14, 2020 at 4:47 AM Finn Thain wrote:
> > Add platform devices for the Mac IDE controller variants. Convert the
> > macide module into a platform driver to support two of those variants.
> > For the third, use a generic "pata_platform" driver instead.
> > This enables automatic loading of the appropriate module and begins
> > the process of replacing the driver with libata alternatives.
> >
> > Cc: Bartlomiej Zolnierkiewicz
> > Cc: Geert Uytterhoeven
> > Cc: Joshua Thompson
> > References: commit 5ed0794cde593 ("m68k/atari: Convert Falcon IDE drivers
> > to platform drivers")
> > References: commit 7ad19a99ad431 ("ide: officially deprecated the legacy
> > IDE driver")
> > Tested-by: Stan Johnson
> > Signed-off-by: Finn Thain
> > ---
> > This patch was tested successfully on a Powerbook 190 (MAC_IDE_BABOON)
> > using both pata_platform and ide_platform drivers.
> > The next step will be to try using these generic drivers with the other
> > IDE controller variants (MAC_IDE_QUADRA or MAC_IDE_PB) so that the macide
> > driver can be entirely replaced with libata drivers.
> >
> > Changed since v1:
> > - Adopted DEFINE_RES_MEM and DEFINE_RES_IRQ macros.
> > - Dropped IORESOURCE_IRQ_SHAREABLE flag as it is ignored by pata_platform.c
> >and IRQF_SHARED makes no difference in this case.
> > - Removed redundant release_mem_region() call.
> > - Enabled CONFIG_BLK_DEV_PLATFORM in mac_defconfig. We might also enable
> >CONFIG_PATA_PLATFORM but IMO migration to libata should be a separate
> >patch (as this patch has some unrelated benefits).
>
> Thanks for the update!
>
> BTW, who do you envision taking this (or the next version)? IDE or m68k?
>
It's unclear from the diff stat. But the focus is on the platform which
probably means it is more interesting to you, as the arch maintainer.
> > --- a/arch/m68k/configs/mac_defconfig
> > +++ b/arch/m68k/configs/mac_defconfig
> > @@ -317,6 +317,7 @@ CONFIG_DUMMY_IRQ=m
> > CONFIG_IDE=y
> > CONFIG_IDE_GD_ATAPI=y
> > CONFIG_BLK_DEV_IDECD=y
> > +CONFIG_BLK_DEV_PLATFORM=y
>
> I guess we want this in multi_defconfig, too?
>
Right. I'll add that in v3.
> > CONFIG_BLK_DEV_MAC_IDE=y
> > CONFIG_RAID_ATTRS=m
> > CONFIG_SCSI=y
>
> > --- a/drivers/ide/macide.c
> > +++ b/drivers/ide/macide.c
>
> > @@ -109,42 +110,61 @@ static const char *mac_ide_name[] =
> > * Probe for a Macintosh IDE interface
> > */
> >
> > -static int __init macide_init(void)
> > +static int mac_ide_probe(struct platform_device *pdev)
> > {
>
> > printk(KERN_INFO "ide: Macintosh %s IDE controller\n",
> > mac_ide_name[macintosh_config->ide_type - 1]);
> >
> > - macide_setup_ports(, base, irq);
> > + macide_setup_ports(, mem->start, irq->start);
> >
> > - return ide_host_add(, hws, 1, NULL);
> > + rc = ide_host_add(, hws, 1, );
> > + if (rc)
> > + return rc;
> > +
> > + platform_set_drvdata(pdev, host);
>
> Move one up, to play it safe?
>
You mean, before calling ide_host_add? The 'host' pointer is uninitialized
prior to that call.
> > + return 0;
> > }
> >
> > -module_init(macide_init);
> > +static int mac_ide_remove(struct platform_device *pdev)
> > +{
> > + struct ide_host *host = dev_get_drvdata(>dev);
>
> As you use platform_set_drvdata() above, you might as well use the
> platform_get_drvdata() helper here, for symmetry.
>
Will do.
Thanks for your review.
> Gr{oetje,eeting}s,
>
> Geert
[PATCH v2] ide/macide: Convert Mac IDE driver to platform driver
Add platform devices for the Mac IDE controller variants. Convert the
macide module into a platform driver to support two of those variants.
For the third, use a generic "pata_platform" driver instead.
This enables automatic loading of the appropriate module and begins
the process of replacing the driver with libata alternatives.
Cc: Bartlomiej Zolnierkiewicz
Cc: Geert Uytterhoeven
Cc: Joshua Thompson
References: commit 5ed0794cde593 ("m68k/atari: Convert Falcon IDE drivers to
platform drivers")
References: commit 7ad19a99ad431 ("ide: officially deprecated the legacy IDE
driver")
Tested-by: Stan Johnson
Signed-off-by: Finn Thain
---
This patch was tested successfully on a Powerbook 190 (MAC_IDE_BABOON)
using both pata_platform and ide_platform drivers.
The next step will be to try using these generic drivers with the other
IDE controller variants (MAC_IDE_QUADRA or MAC_IDE_PB) so that the macide
driver can be entirely replaced with libata drivers.
Changed since v1:
- Adopted DEFINE_RES_MEM and DEFINE_RES_IRQ macros.
- Dropped IORESOURCE_IRQ_SHAREABLE flag as it is ignored by pata_platform.c
and IRQF_SHARED makes no difference in this case.
- Removed redundant release_mem_region() call.
- Enabled CONFIG_BLK_DEV_PLATFORM in mac_defconfig. We might also enable
CONFIG_PATA_PLATFORM but IMO migration to libata should be a separate
patch (as this patch has some unrelated benefits).
---
arch/m68k/configs/mac_defconfig | 1 +
arch/m68k/mac/config.c | 41
drivers/ide/macide.c| 66 +
3 files changed, 85 insertions(+), 23 deletions(-)
diff --git a/arch/m68k/configs/mac_defconfig b/arch/m68k/configs/mac_defconfig
index 6087798662601..f770970fe4e99 100644
--- a/arch/m68k/configs/mac_defconfig
+++ b/arch/m68k/configs/mac_defconfig
@@ -317,6 +317,7 @@ CONFIG_DUMMY_IRQ=m
CONFIG_IDE=y
CONFIG_IDE_GD_ATAPI=y
CONFIG_BLK_DEV_IDECD=y
+CONFIG_BLK_DEV_PLATFORM=y
CONFIG_BLK_DEV_MAC_IDE=y
CONFIG_RAID_ATTRS=m
CONFIG_SCSI=y
diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
index 5c9f3a2d65388..43fc29180cb58 100644
--- a/arch/m68k/mac/config.c
+++ b/arch/m68k/mac/config.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -940,6 +941,26 @@ static const struct resource mac_scsi_ccl_rsrc[]
__initconst = {
},
};
+static const struct resource mac_ide_quadra_rsrc[] __initconst = {
+ DEFINE_RES_MEM(0x50F1A000, 0x104),
+ DEFINE_RES_IRQ(IRQ_NUBUS_F),
+};
+
+static const struct resource mac_ide_pb_rsrc[] __initconst = {
+ DEFINE_RES_MEM(0x50F1A000, 0x104),
+ DEFINE_RES_IRQ(IRQ_NUBUS_C),
+};
+
+static const struct resource mac_pata_baboon_rsrc[] __initconst = {
+ DEFINE_RES_MEM(0x50F1A000, 0x38),
+ DEFINE_RES_MEM(0x50F1A038, 0x04),
+ DEFINE_RES_IRQ(IRQ_BABOON_1),
+};
+
+static const struct pata_platform_info mac_pata_baboon_data __initconst = {
+ .ioport_shift = 2,
+};
+
int __init mac_platform_init(void)
{
phys_addr_t swim_base = 0;
@@ -1048,6 +1069,26 @@ int __init mac_platform_init(void)
break;
}
+ /*
+* IDE device
+*/
+
+ switch (macintosh_config->ide_type) {
+ case MAC_IDE_QUADRA:
+ platform_device_register_simple("mac_ide", -1,
+ mac_ide_quadra_rsrc, ARRAY_SIZE(mac_ide_quadra_rsrc));
+ break;
+ case MAC_IDE_PB:
+ platform_device_register_simple("mac_ide", -1,
+ mac_ide_pb_rsrc, ARRAY_SIZE(mac_ide_pb_rsrc));
+ break;
+ case MAC_IDE_BABOON:
+ platform_device_register_resndata(NULL, "pata_platform", -1,
+ mac_pata_baboon_rsrc, ARRAY_SIZE(mac_pata_baboon_rsrc),
+ _pata_baboon_data, sizeof(mac_pata_baboon_data));
+ break;
+ }
+
/*
* Ethernet device
*/
diff --git a/drivers/ide/macide.c b/drivers/ide/macide.c
index 3c6bb8599303b..f2236456b3706 100644
--- a/drivers/ide/macide.c
+++ b/drivers/ide/macide.c
@@ -18,10 +18,11 @@
#include
#include
#include
+#include
#include
-#include
-#include
+
+#define DRV_NAME "mac_ide"
#define IDE_BASE 0x50F1A000/* Base address of IDE controller */
@@ -109,42 +110,61 @@ static const char *mac_ide_name[] =
* Probe for a Macintosh IDE interface
*/
-static int __init macide_init(void)
+static int mac_ide_probe(struct platform_device *pdev)
{
- unsigned long base;
- int irq;
+ struct resource *mem, *irq;
struct ide_hw hw, *hws[] = { };
struct ide_port_info d = macide_port_info;
+ struct ide_host *host;
+ int rc;
if (!MACH_IS_MAC)
return -ENODEV;
- switch (macintosh_config->ide_type) {
- case MAC_IDE_QUADRA:
-
Re: [PATCH] ide/macide: Convert Mac IDE driver to platform driver
On Fri, 11 Sep 2020, Geert Uytterhoeven wrote:
>
> Sorry, I completely missed that the Baboon case registers a
> "pata_platform" device instead of a "macide" device. Hence please
> ignore my comments related to that. Sorry for the noise.
>
No problem. That misunderstanding got me thinking about implications
stemming from my patch that I may have overlooked. Here's what I found.
1) Your presumption that the old macide driver would keep supporting all
variants does make sense, as that would delay disruption for as long as
possible (i.e. for as long as the IDE subsystem remains).
However, if my patch does not get merged until 2021, that disruption would
not arrive earlier than promised by commit 7ad19a99ad431 ("ide: officially
deprecated the legacy IDE driver").
2) My patch omitted a mac_defconfig update that would enable an
alternative driver for the Baboon case. I will remedy this in v2.
3) It turns out that the Debian/m68k kernel config has
CONFIG_BLK_DEV_PLATFORM=m. This will work fine with this patch. (I assume
that Debian developers will replace CONFIG_BLK_DEV_PLATFORM with
CONFIG_PATA_PLATFORM prior to the removal of the IDE subsystem next year.)
Re: [PATCH] ide/macide: Convert Mac IDE driver to platform driver
On Thu, 10 Sep 2020, Geert Uytterhoeven wrote:
> On Thu, Sep 10, 2020 at 2:23 AM Finn Thain wrote:
> > I prefer a declarative or data-driven style, even if it takes a few
> > more lines of code. But there is a compromise:
> >
> > static const struct resource mac_ide_quadra_rsrc[] __initconst = {
> > DEFINE_RES_MEM(0x50F1A000, 0x104),
> > DEFINE_RES_IRQ(IRQ_NUBUS_F),
> > }
> >
> > static const struct resource mac_ide_pb_rsrc[] __initconst = {
> > DEFINE_RES_MEM(0x50F1A000, 0x104),
> > DEFINE_RES_IRQ(IRQ_NUBUS_C),
> > }
> >
> > The reason I didn't use these macros was to avoid making the reader go and
> > look up their definitions. Anyway, would that style be preferred here?
>
> I think the DEFINE_RES_*() are sufficiently common (well, in pre-DT
> platforms ;-)
>
OK, I'll use the macros in v2.
> > I could do the same with the mac_ide_baboon_rsrc[] initializer:
> >
> > static const struct resource mac_pata_baboon_rsrc[] __initconst = {
> > DEFINE_RES_MEM(0x50F1A000, 0x38),
> > DEFINE_RES_MEM(0x50F1A038, 0x04),
> > DEFINE_RES_IRQ(IRQ_BABOON_1),
> > };
> >
> > ... but that would lose the IORESOURCE_IRQ_SHAREABLE flag. I'm not sure
> > whether that matters (it's a vestige of macide.c).
>
> You can still use DEFINE_RES_NAMED() to pass the flags.
> Would you consider that to be a good compromise?
>
Sure. I was happy with v1. I'm not that worried about brevity.
Anyway, the question remains, is the flag actually needed. I can't see a
need for it so I'll omit the flag in v2 and ask Stan to test again.
> > > > +static const struct resource mac_pata_baboon_rsrc[] __initconst = {
> > > > + {
> > > > + .flags = IORESOURCE_MEM,
> > > > + .start = 0x50F1A000,
> > > > + .end = 0x50F1A037,
> > > > + }, {
> > > > + .flags = IORESOURCE_MEM,
> > > > + .start = 0x50F1A038,
> > > > + .end = 0x50F1A03B,
> > > > + }, {
> > > > + .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_SHAREABLE,
> > > > + .start = IRQ_BABOON_1,
> > > > + .end = IRQ_BABOON_1,
> > > > + },
> > > > +};
> > > > +
> > > > +static const struct pata_platform_info mac_pata_baboon_data
> > > > __initconst = {
> > > > + .ioport_shift = 2,
> > > > +};
> > >
> > > Just wondering: how is this implemented in drivers/ide/macide.c, which
> > > doesn't use the platform info?
> >
> > That factor of 4 is embedded in the address caclulation:
> >
> > for (i = 0; i < 8; i++)
> > hw->io_ports_array[i] = base + i * 4;
>
> IC. But in the new code, the platform info is passed for Baboon only,
> while the old code used it for all variants.
>
Correct. Is that a problem for some reason?
> > > > --- a/drivers/ide/macide.c
> > > > +++ b/drivers/ide/macide.c
> > > > @@ -18,10 +18,11 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > >
> > > > #include
> > > > -#include
> > > > -#include
> > > > +
> > > > +#define DRV_NAME "mac_ide"
> > > >
> > > > #define IDE_BASE 0x50F1A000/* Base address of IDE controller */
> > >
> > > Do you still need this definition?
> > > Yes, because it's still used to access IDE_IFR.
> > > Ideally, that should be converted to use the base from the resource,
> > > too.
> > >
> >
> > Yes, that was my thought too. I can make the change if you like, but I
> > can't test it until I set up the appropriate hardware (MAC_IDE_QUADRA or
> > MAC_IDE_PB). I do own that hardware but it is located in Melbourne and it
> > is now illegal to visit Melbourne without official papers. Besides, once I
> > can test on that hardware I can replace the entire driver anyway, and
> > this kind of refactoring would become moot.
>
> OK.
>
> > > > @@ -109,42 +110,65 @@ static const char *mac_ide_name[] =
> > > > * Probe for a Macintosh IDE interface
> > > > */
> > > >
> > > > -static int __init macide_init(void)
> > > > +static int mac_ide_probe(struct platform_device *pdev)
> > > > {
> > > > - unsigned long base;
Re: [PATCH] ide/macide: Convert Mac IDE driver to platform driver
On Wed, 9 Sep 2020, Geert Uytterhoeven wrote:
>
> Thanks for your patch!
>
Thanks for your review.
> > --- a/arch/m68k/mac/config.c +++ b/arch/m68k/mac/config.c
>
> > @@ -940,6 +941,50 @@ static const struct resource mac_scsi_ccl_rsrc[]
> > __initconst = {
> > },
> > };
> >
> > +static const struct resource mac_ide_quadra_rsrc[] __initconst = {
> > + {
> > + .flags = IORESOURCE_MEM,
> > + .start = 0x50F1A000,
> > + .end = 0x50F1A103,
> > + }, {
> > + .flags = IORESOURCE_IRQ,
> > + .start = IRQ_NUBUS_F,
> > + .end = IRQ_NUBUS_F,
> > + },
> > +};
> > +
> > +static const struct resource mac_ide_pb_rsrc[] __initconst = {
> > + {
> > + .flags = IORESOURCE_MEM,
> > + .start = 0x50F1A000,
> > + .end = 0x50F1A103,
> > + }, {
> > + .flags = IORESOURCE_IRQ,
> > + .start = IRQ_NUBUS_C,
> > + .end = IRQ_NUBUS_C,
> > + },
> > +};
>
> As the above two variants are almost identical, perhaps it makes sense
> to drop one of them, drop the const, and override the irq values
> dynamically?
>
I prefer a declarative or data-driven style, even if it takes a few more
lines of code. But there is a compromise:
static const struct resource mac_ide_quadra_rsrc[] __initconst = {
DEFINE_RES_MEM(0x50F1A000, 0x104),
DEFINE_RES_IRQ(IRQ_NUBUS_F),
}
static const struct resource mac_ide_pb_rsrc[] __initconst = {
DEFINE_RES_MEM(0x50F1A000, 0x104),
DEFINE_RES_IRQ(IRQ_NUBUS_C),
}
The reason I didn't use these macros was to avoid making the reader go and
look up their definitions. Anyway, would that style be preferred here?
I could do the same with the mac_ide_baboon_rsrc[] initializer:
static const struct resource mac_pata_baboon_rsrc[] __initconst = {
DEFINE_RES_MEM(0x50F1A000, 0x38),
DEFINE_RES_MEM(0x50F1A038, 0x04),
DEFINE_RES_IRQ(IRQ_BABOON_1),
};
... but that would lose the IORESOURCE_IRQ_SHAREABLE flag. I'm not sure
whether that matters (it's a vestige of macide.c).
> > +
> > +static const struct resource mac_pata_baboon_rsrc[] __initconst = {
> > + {
> > + .flags = IORESOURCE_MEM,
> > + .start = 0x50F1A000,
> > + .end = 0x50F1A037,
> > + }, {
> > + .flags = IORESOURCE_MEM,
> > + .start = 0x50F1A038,
> > + .end = 0x50F1A03B,
> > + }, {
> > + .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_SHAREABLE,
> > + .start = IRQ_BABOON_1,
> > + .end = IRQ_BABOON_1,
> > + },
> > +};
> > +
> > +static const struct pata_platform_info mac_pata_baboon_data __initconst = {
> > + .ioport_shift = 2,
> > +};
>
> Just wondering: how is this implemented in drivers/ide/macide.c, which
> doesn't use the platform info?
>
That factor of 4 is embedded in the address caclulation:
for (i = 0; i < 8; i++)
hw->io_ports_array[i] = base + i * 4;
> > --- a/drivers/ide/macide.c
> > +++ b/drivers/ide/macide.c
> > @@ -18,10 +18,11 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > -#include
> > -#include
> > +
> > +#define DRV_NAME "mac_ide"
> >
> > #define IDE_BASE 0x50F1A000/* Base address of IDE controller */
>
> Do you still need this definition?
> Yes, because it's still used to access IDE_IFR.
> Ideally, that should be converted to use the base from the resource,
> too.
>
Yes, that was my thought too. I can make the change if you like, but I
can't test it until I set up the appropriate hardware (MAC_IDE_QUADRA or
MAC_IDE_PB). I do own that hardware but it is located in Melbourne and it
is now illegal to visit Melbourne without official papers. Besides, once I
can test on that hardware I can replace the entire driver anyway, and
this kind of refactoring would become moot.
> >
> > @@ -109,42 +110,65 @@ static const char *mac_ide_name[] =
> > * Probe for a Macintosh IDE interface
> > */
> >
> > -static int __init macide_init(void)
> > +static int mac_ide_probe(struct platform_device *pdev)
> > {
> > - unsigned long base;
> > - int irq;
> > + struct resource *mem, *irq;
> > struct ide_hw hw, *hws[] = { };
> > struct ide_port_info d = macide_port_info;
> > + struct ide_host *host;
> > + int rc;
> >
> > if (!MACH_IS_MAC)
> > return -ENODEV;
> >
> > - switch (macintosh_config->ide_type) {
> > - case MAC_IDE_QUADRA:
> > - base = IDE_BASE;
> > - irq = IRQ_NUBUS_F;
> > - break;
> > - case MAC_IDE_PB:
> > - base = IDE_BASE;
> > - irq = IRQ_NUBUS_C;
> > - break;
> > - case MAC_IDE_BABOON:
> > - base = BABOON_BASE;
> > - d.port_ops = NULL;
>
>
[PATCH 1/5] powerpc/tau: Use appropriate temperature sample interval
According to the MPC750 Users Manual, the SITV value in Thermal
Management Register 3 is 13 bits long. The present code calculates the
SITV value as 60 * 500 cycles. This would overflow to give 10 us on
a 500 MHz CPU rather than the intended 60 us. (But according to the
Microprocessor Datasheet, there is also a factor of 266 that has to be
applied to this value on certain parts i.e. speed sort above 266 MHz.)
Always use the maximum cycle count, as recommended by the Datasheet.
Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2")
Tested-by: Stan Johnson
Signed-off-by: Finn Thain
---
arch/powerpc/include/asm/reg.h | 2 +-
arch/powerpc/kernel/tau_6xx.c | 12
2 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 88e6c78100d9b..c750afc62887c 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -815,7 +815,7 @@
#define THRM1_TIN (1 << 31)
#define THRM1_TIV (1 << 30)
#define THRM1_THRES(x) ((x&0x7f)<<23)
-#define THRM3_SITV(x) ((x&0x3fff)<<1)
+#define THRM3_SITV(x) ((x & 0x1fff) << 1)
#define THRM1_TID (1<<2)
#define THRM1_TIE (1<<1)
#define THRM1_V(1<<0)
diff --git a/arch/powerpc/kernel/tau_6xx.c b/arch/powerpc/kernel/tau_6xx.c
index e2ab8a111b693..976d5bc1b5176 100644
--- a/arch/powerpc/kernel/tau_6xx.c
+++ b/arch/powerpc/kernel/tau_6xx.c
@@ -178,15 +178,11 @@ static void tau_timeout(void * info)
* complex sleep code needs to be added. One mtspr every time
* tau_timeout is called is probably not a big deal.
*
-* Enable thermal sensor and set up sample interval timer
-* need 20 us to do the compare.. until a nice 'cpu_speed' function
-* call is implemented, just assume a 500 mhz clock. It doesn't really
-* matter if we take too long for a compare since it's all interrupt
-* driven anyway.
-*
-* use a extra long time.. (60 us @ 500 mhz)
+* The "PowerPC 740 and PowerPC 750 Microprocessor Datasheet"
+* recommends that "the maximum value be set in THRM3 under all
+* conditions."
*/
- mtspr(SPRN_THRM3, THRM3_SITV(500*60) | THRM3_E);
+ mtspr(SPRN_THRM3, THRM3_SITV(0x1fff) | THRM3_E);
local_irq_restore(flags);
}
--
2.26.2
[PATCH 0/5] powerpc/tau: TAU driver fixes
This patch series fixes various bugs in the Thermal Assist Unit driver. It was tested on 266 MHz and 292 MHz PowerBook G3 laptops. Finn Thain (5): powerpc/tau: Use appropriate temperature sample interval powerpc/tau: Convert from timer to workqueue powerpc/tau: Remove duplicated set_thresholds() call powerpc/tau: Check processor type before enabling TAU interrupt powerpc/tau: Disable TAU between measurements arch/powerpc/include/asm/reg.h | 2 +- arch/powerpc/kernel/tau_6xx.c | 147 + arch/powerpc/platforms/Kconfig | 14 +--- 3 files changed, 62 insertions(+), 101 deletions(-) -- 2.26.2
[PATCH 3/5] powerpc/tau: Remove duplicated set_thresholds() call
The commentary at the call site seems to disagree with the code. The
conditional prevents calling set_thresholds() via the exception handler,
which appears to crash. Perhaps that's because it immediately triggers
another TAU exception. Anyway, calling set_thresholds() from TAUupdate()
is redundant because tau_timeout() does so.
Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2")
Tested-by: Stan Johnson
Signed-off-by: Finn Thain
---
arch/powerpc/kernel/tau_6xx.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/arch/powerpc/kernel/tau_6xx.c b/arch/powerpc/kernel/tau_6xx.c
index 268205cc347da..b8d7e7d498e0a 100644
--- a/arch/powerpc/kernel/tau_6xx.c
+++ b/arch/powerpc/kernel/tau_6xx.c
@@ -110,11 +110,6 @@ static void TAUupdate(int cpu)
#ifdef DEBUG
printk("grew = %d\n", tau[cpu].grew);
#endif
-
-#ifndef CONFIG_TAU_INT /* tau_timeout will do this if not using interrupts */
- set_thresholds(cpu);
-#endif
-
}
#ifdef CONFIG_TAU_INT
--
2.26.2
[PATCH 2/5] powerpc/tau: Convert from timer to workqueue
Since commit 19dbdcb8039cf ("smp: Warn on function calls from softirq
context") the Thermal Assist Unit driver causes a warning like the
following when CONFIG_SMP is enabled.
[ cut here ]
WARNING: CPU: 0 PID: 0 at kernel/smp.c:428
smp_call_function_many_cond+0xf4/0x38c
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.7.0-pmac #3
NIP: c00b37a8 LR: c00b3abc CTR: c001218c
REGS: c0799c60 TRAP: 0700 Not tainted (5.7.0-pmac)
MSR: 00029032 CR: 42000224 XER:
GPR00: c00b3abc c0799d18 c076e300 c079ef5c c0011fec
GPR08: 0100 0100 8000 42000224 c079d040 c079d044
GPR16: 0001 0004 c0799da0 c079f054 c07a c07a
GPR24: c0011fec c079ef5c c079ef5c
NIP [c00b37a8] smp_call_function_many_cond+0xf4/0x38c
LR [c00b3abc] on_each_cpu+0x38/0x68
Call Trace:
[c0799d18] [] 0x (unreliable)
[c0799d68] [c00b3abc] on_each_cpu+0x38/0x68
[c0799d88] [c0096704] call_timer_fn.isra.26+0x20/0x7c
[c0799d98] [c0096b40] run_timer_softirq+0x1d4/0x3fc
[c0799df8] [c05b4368] __do_softirq+0x118/0x240
[c0799e58] [c0039c44] irq_exit+0xc4/0xcc
[c0799e68] [c000ade8] timer_interrupt+0x1b0/0x230
[c0799ea8] [c0013520] ret_from_except+0x0/0x14
--- interrupt: 901 at arch_cpu_idle+0x24/0x6c
LR = arch_cpu_idle+0x24/0x6c
[c0799f70] [0001] 0x1 (unreliable)
[c0799f80] [c0060990] do_idle+0xd8/0x17c
[c0799fa0] [c0060ba8] cpu_startup_entry+0x24/0x28
[c0799fb0] [c072d220] start_kernel+0x434/0x44c
[c0799ff0] [3860] 0x3860
Instruction dump:
8129f204 2f89 40beff98 3d20c07a 8929eec4 2f89 40beff88 0fe0
8122 552805de 550802ef 4182ff84 <0fe0> 3860 7f65db78 7f44d378
---[ end trace 34a886e47819c2eb ]---
Don't call on_each_cpu() from a timer callback, call it from a worker
thread instead.
Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2")
Tested-by: Stan Johnson
Signed-off-by: Finn Thain
---
arch/powerpc/kernel/tau_6xx.c | 38 +--
1 file changed, 18 insertions(+), 20 deletions(-)
diff --git a/arch/powerpc/kernel/tau_6xx.c b/arch/powerpc/kernel/tau_6xx.c
index 976d5bc1b5176..268205cc347da 100644
--- a/arch/powerpc/kernel/tau_6xx.c
+++ b/arch/powerpc/kernel/tau_6xx.c
@@ -13,13 +13,14 @@
*/
#include
-#include
#include
#include
#include
#include
#include
#include
+#include
+#include
#include
#include
@@ -39,8 +40,6 @@ static struct tau_temp
unsigned char grew;
} tau[NR_CPUS];
-struct timer_list tau_timer;
-
#undef DEBUG
/* TODO: put these in a /proc interface, with some sanity checks, and maybe
@@ -50,7 +49,7 @@ struct timer_list tau_timer;
#define step_size 2 /* step size when temp goes out of
range */
#define window_expand 1 /* expand the window by this much */
/* configurable values for shrinking the window */
-#define shrink_timer 2*HZ/* period between shrinking the window */
+#define shrink_timer 2000/* period between shrinking the window */
#define min_window 2 /* minimum window size, degrees C */
static void set_thresholds(unsigned long cpu)
@@ -187,14 +186,18 @@ static void tau_timeout(void * info)
local_irq_restore(flags);
}
-static void tau_timeout_smp(struct timer_list *unused)
-{
+static struct workqueue_struct *tau_workq;
- /* schedule ourselves to be run again */
- mod_timer(_timer, jiffies + shrink_timer) ;
+static void tau_work_func(struct work_struct *work)
+{
+ msleep(shrink_timer);
on_each_cpu(tau_timeout, NULL, 0);
+ /* schedule ourselves to be run again */
+ queue_work(tau_workq, work);
}
+DECLARE_WORK(tau_work, tau_work_func);
+
/*
* setup the TAU
*
@@ -227,21 +230,16 @@ static int __init TAU_init(void)
return 1;
}
-
- /* first, set up the window shrinking timer */
- timer_setup(_timer, tau_timeout_smp, 0);
- tau_timer.expires = jiffies + shrink_timer;
- add_timer(_timer);
+ tau_workq = alloc_workqueue("tau", WQ_UNBOUND, 1, 0);
+ if (!tau_workq)
+ return -ENOMEM;
on_each_cpu(TAU_init_smp, NULL, 0);
- printk("Thermal assist unit ");
-#ifdef CONFIG_TAU_INT
- printk("using interrupts, ");
-#else
- printk("using timers, ");
-#endif
- printk("shrink_timer: %d jiffies\n", shrink_timer);
+ queue_work(tau_workq, _work);
+
+ pr_info("Thermal assist unit using %s, shrink_timer: %d ms\n",
+ IS_ENABLED(CONFIG_TAU_INT) ? "interrupts" : "workqueue",
shrink_timer);
tau_initialized = 1;
return 0;
--
2.26.2
[PATCH 5/5] powerpc/tau: Disable TAU between measurements
Enabling CONFIG_TAU_INT causes random crashes:
Unrecoverable exception 1700 at c0009414 (msr=1000)
Oops: Unrecoverable exception, sig: 6 [#1]
BE PAGE_SIZE=4K MMU=Hash SMP NR_CPUS=2 PowerMac
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.7.0-pmac-00043-gd5f545e1a8593 #5
NIP: c0009414 LR: c0009414 CTR: c00116fc
REGS: c0799eb8 TRAP: 1700 Not tainted (5.7.0-pmac-00043-gd5f545e1a8593)
MSR: 1000 CR: 22000228 XER: 0100
GPR00: c0799f70 c076e300 0080 0291c0ac 00e0 c076e300 00049032
GPR08: 0001 c00116fc dfbd3200 007f80a8
GPR16: c075ce04
GPR24: c075ce04 dfff8880 c07b c075ce04 0008 0001 c079ef98 c079ef5c
NIP [c0009414] arch_cpu_idle+0x24/0x6c
LR [c0009414] arch_cpu_idle+0x24/0x6c
Call Trace:
[c0799f70] [0001] 0x1 (unreliable)
[c0799f80] [c0060990] do_idle+0xd8/0x17c
[c0799fa0] [c0060ba4] cpu_startup_entry+0x20/0x28
[c0799fb0] [c072d220] start_kernel+0x434/0x44c
[c0799ff0] [3860] 0x3860
Instruction dump:
3d20c07b 7c0802a6
4e800421 7d2000a6
---[ end trace 3a0c9b5cb216db6b ]---
Resolve this problem by disabling each THRMn comparator when handling
the associated THRMn interrupt and by disabling the TAU entirely when
updating THRMn thresholds.
Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2")
Tested-by: Stan Johnson
Signed-off-by: Finn Thain
---
arch/powerpc/kernel/tau_6xx.c | 65 +-
arch/powerpc/platforms/Kconfig | 9 ++---
2 files changed, 26 insertions(+), 48 deletions(-)
diff --git a/arch/powerpc/kernel/tau_6xx.c b/arch/powerpc/kernel/tau_6xx.c
index 614b5b272d9c6..0b4694b8d2482 100644
--- a/arch/powerpc/kernel/tau_6xx.c
+++ b/arch/powerpc/kernel/tau_6xx.c
@@ -42,8 +42,6 @@ static struct tau_temp
static bool tau_int_enable;
-#undef DEBUG
-
/* TODO: put these in a /proc interface, with some sanity checks, and maybe
* dynamic adjustment to minimize # of interrupts */
/* configurable values for step size and how much to expand the window when
@@ -67,42 +65,33 @@ static void set_thresholds(unsigned long cpu)
static void TAUupdate(int cpu)
{
- unsigned thrm;
-
-#ifdef DEBUG
- printk("TAUupdate ");
-#endif
+ u32 thrm;
+ u32 bits = THRM1_TIV | THRM1_TIN | THRM1_V;
/* if both thresholds are crossed, the step_sizes cancel out
* and the window winds up getting expanded twice. */
- if((thrm = mfspr(SPRN_THRM1)) & THRM1_TIV){ /* is valid? */
- if(thrm & THRM1_TIN){ /* crossed low threshold */
- if (tau[cpu].low >= step_size){
- tau[cpu].low -= step_size;
- tau[cpu].high -= (step_size - window_expand);
- }
- tau[cpu].grew = 1;
-#ifdef DEBUG
- printk("low threshold crossed ");
-#endif
+ thrm = mfspr(SPRN_THRM1);
+ if ((thrm & bits) == bits) {
+ mtspr(SPRN_THRM1, 0);
+
+ if (tau[cpu].low >= step_size) {
+ tau[cpu].low -= step_size;
+ tau[cpu].high -= (step_size - window_expand);
}
+ tau[cpu].grew = 1;
+ pr_debug("%s: low threshold crossed\n", __func__);
}
- if((thrm = mfspr(SPRN_THRM2)) & THRM1_TIV){ /* is valid? */
- if(thrm & THRM1_TIN){ /* crossed high threshold */
- if (tau[cpu].high <= 127-step_size){
- tau[cpu].low += (step_size - window_expand);
- tau[cpu].high += step_size;
- }
- tau[cpu].grew = 1;
-#ifdef DEBUG
- printk("high threshold crossed ");
-#endif
+ thrm = mfspr(SPRN_THRM2);
+ if ((thrm & bits) == bits) {
+ mtspr(SPRN_THRM2, 0);
+
+ if (tau[cpu].high <= 127 - step_size) {
+ tau[cpu].low += (step_size - window_expand);
+ tau[cpu].high += step_size;
}
+ tau[cpu].grew = 1;
+ pr_debug("%s: high threshold crossed\n", __func__);
}
-
-#ifdef DEBUG
- printk("grew = %d\n", tau[cpu].grew);
-#endif
}
#ifdef CONFIG_TAU_INT
@@ -127,17 +116,17 @@ void TAUException(struct pt_regs * regs)
static void tau_timeout(void * info)
{
int cpu;
- unsigned long flags;
int size;
int shrink;
- /* disabling interrupts *should* be okay */
- local_irq_save(flags);
cpu = smp_processor_id();
if (!tau_int_enable)
TAUupdate(cpu);
+ /* Stop thermal sensor comparisons and interrupts */
[PATCH 4/5] powerpc/tau: Check processor type before enabling TAU interrupt
According to Freescale's documentation, MPC74XX processors have an
erratum that prevents the TAU interrupt from working, so don't try to
use it when running on those processors.
Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2")
Tested-by: Stan Johnson
Signed-off-by: Finn Thain
---
arch/powerpc/kernel/tau_6xx.c | 33 ++---
arch/powerpc/platforms/Kconfig | 5 ++---
2 files changed, 16 insertions(+), 22 deletions(-)
diff --git a/arch/powerpc/kernel/tau_6xx.c b/arch/powerpc/kernel/tau_6xx.c
index b8d7e7d498e0a..614b5b272d9c6 100644
--- a/arch/powerpc/kernel/tau_6xx.c
+++ b/arch/powerpc/kernel/tau_6xx.c
@@ -40,6 +40,8 @@ static struct tau_temp
unsigned char grew;
} tau[NR_CPUS];
+static bool tau_int_enable;
+
#undef DEBUG
/* TODO: put these in a /proc interface, with some sanity checks, and maybe
@@ -54,22 +56,13 @@ static struct tau_temp
static void set_thresholds(unsigned long cpu)
{
-#ifdef CONFIG_TAU_INT
- /*
-* setup THRM1,
-* threshold, valid bit, enable interrupts, interrupt when below
threshold
-*/
- mtspr(SPRN_THRM1, THRM1_THRES(tau[cpu].low) | THRM1_V | THRM1_TIE |
THRM1_TID);
+ u32 maybe_tie = tau_int_enable ? THRM1_TIE : 0;
- /* setup THRM2,
-* threshold, valid bit, enable interrupts, interrupt when above
threshold
-*/
- mtspr (SPRN_THRM2, THRM1_THRES(tau[cpu].high) | THRM1_V | THRM1_TIE);
-#else
- /* same thing but don't enable interrupts */
- mtspr(SPRN_THRM1, THRM1_THRES(tau[cpu].low) | THRM1_V | THRM1_TID);
- mtspr(SPRN_THRM2, THRM1_THRES(tau[cpu].high) | THRM1_V);
-#endif
+ /* setup THRM1, threshold, valid bit, interrupt when below threshold */
+ mtspr(SPRN_THRM1, THRM1_THRES(tau[cpu].low) | THRM1_V | maybe_tie |
THRM1_TID);
+
+ /* setup THRM2, threshold, valid bit, interrupt when above threshold */
+ mtspr(SPRN_THRM2, THRM1_THRES(tau[cpu].high) | THRM1_V | maybe_tie);
}
static void TAUupdate(int cpu)
@@ -142,9 +135,8 @@ static void tau_timeout(void * info)
local_irq_save(flags);
cpu = smp_processor_id();
-#ifndef CONFIG_TAU_INT
- TAUupdate(cpu);
-#endif
+ if (!tau_int_enable)
+ TAUupdate(cpu);
size = tau[cpu].high - tau[cpu].low;
if (size > min_window && ! tau[cpu].grew) {
@@ -225,6 +217,9 @@ static int __init TAU_init(void)
return 1;
}
+ tau_int_enable = IS_ENABLED(CONFIG_TAU_INT) &&
+!strcmp(cur_cpu_spec->platform, "ppc750");
+
tau_workq = alloc_workqueue("tau", WQ_UNBOUND, 1, 0);
if (!tau_workq)
return -ENOMEM;
@@ -234,7 +229,7 @@ static int __init TAU_init(void)
queue_work(tau_workq, _work);
pr_info("Thermal assist unit using %s, shrink_timer: %d ms\n",
- IS_ENABLED(CONFIG_TAU_INT) ? "interrupts" : "workqueue",
shrink_timer);
+ tau_int_enable ? "interrupts" : "workqueue", shrink_timer);
tau_initialized = 1;
return 0;
diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
index fb7515b4fa9c6..9fe36f0b54c1a 100644
--- a/arch/powerpc/platforms/Kconfig
+++ b/arch/powerpc/platforms/Kconfig
@@ -223,9 +223,8 @@ config TAU
temperature within 2-4 degrees Celsius. This option shows the current
on-die temperature in /proc/cpuinfo if the cpu supports it.
- Unfortunately, on some chip revisions, this sensor is very inaccurate
- and in many cases, does not work at all, so don't assume the cpu
- temp is actually what /proc/cpuinfo says it is.
+ Unfortunately, this sensor is very inaccurate when uncalibrated, so
+ don't assume the cpu temp is actually what /proc/cpuinfo says it is.
config TAU_INT
bool "Interrupt driven TAU driver (DANGEROUS)"
--
2.26.2
Re: [PATCH 17/19] z2ram: reindent
On Thu, 27 Aug 2020, Joe Perches wrote: > > checkpatch already does this. > Did you use checkpatch to generate this patch?
Re: [PATCH 17/19] z2ram: reindent
> @@ -109,34 +111,28 @@ static void get_z2ram(void)
...
> }
> -
> - return;
> }
>
It would be good to have a semantic patch for that change.
>
> - if (z2ram_map[z2ram_size] == 0) {
> + if (z2ram_map[z2ram_size] == 0)
> break;
> - }
>
And that one.
Reason being, a semantic patch only has to be debugged once, whereas
manual churn has to be done correctly over and over again.
TBH, this kind of transformation can happen when code is displayed.
It doesn't have to involve git at all. Otherwise, why have 5 billion
transistors? You'd be better off with an Amiga.
Re: [PATCH] m68k: Correct some typos in comments
On Wed, 26 Aug 2020, Geert Uytterhoeven wrote:
> > @@ -2654,7 +2654,7 @@ func_startmmu_get_page_table_entry,%d0/%a1
> > jne 2f
> >
> > /* If the page table entry doesn't exist, we allocate a complete new
> > -* page and use it as one continues big page table which can cover
> > +* page and use it as one continuous big page table which can cover
>
> Usually I use "contiguous", but the dictionary seems to permit both.
>
Two or more things may be said to be "contiguous". I think this sentence
refers to only one thing.
> > * 4MB of memory, nearly almost all mappings have that alignment.
> > */
> > get_new_page
>
> Reviewed-by: Geert Uytterhoeven
> i.e. will queue in the m68k for-v5.10 branch.
>
Thanks.
> Gr{oetje,eeting}s,
>
> Geert
>
[PATCH] ide/macide: Convert Mac IDE driver to platform driver
Add platform devices for the Mac IDE controller variants. Convert the
macide module into a platform driver to support two of those variants.
For the third, use a generic "pata_platform" driver instead.
This enables automatic loading of the appropriate module and begins
the process of replacing the driver with libata alternatives.
Cc: Bartlomiej Zolnierkiewicz
Cc: Geert Uytterhoeven
Cc: Joshua Thompson
References: commit 5ed0794cde593 ("m68k/atari: Convert Falcon IDE drivers to
platform drivers")
References: commit 7ad19a99ad431 ("ide: officially deprecated the legacy IDE
driver")
Tested-by: Stan Johnson
Signed-off-by: Finn Thain
---
This patch was tested successfully on a Powerbook 190 (MAC_IDE_BABOON)
using both pata_platform and ide_platform drivers.
The next step will be to try using these generic drivers with the other
IDE controller variants (MAC_IDE_QUADRA or MAC_IDE_PB) so that the macide
driver can be entirely replaced with libata drivers.
---
arch/m68k/mac/config.c | 65 +++
drivers/ide/macide.c | 70 --
2 files changed, 112 insertions(+), 23 deletions(-)
diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
index 5c9f3a2d65388..9ad3a0d3481b1 100644
--- a/arch/m68k/mac/config.c
+++ b/arch/m68k/mac/config.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -940,6 +941,50 @@ static const struct resource mac_scsi_ccl_rsrc[]
__initconst = {
},
};
+static const struct resource mac_ide_quadra_rsrc[] __initconst = {
+ {
+ .flags = IORESOURCE_MEM,
+ .start = 0x50F1A000,
+ .end = 0x50F1A103,
+ }, {
+ .flags = IORESOURCE_IRQ,
+ .start = IRQ_NUBUS_F,
+ .end = IRQ_NUBUS_F,
+ },
+};
+
+static const struct resource mac_ide_pb_rsrc[] __initconst = {
+ {
+ .flags = IORESOURCE_MEM,
+ .start = 0x50F1A000,
+ .end = 0x50F1A103,
+ }, {
+ .flags = IORESOURCE_IRQ,
+ .start = IRQ_NUBUS_C,
+ .end = IRQ_NUBUS_C,
+ },
+};
+
+static const struct resource mac_pata_baboon_rsrc[] __initconst = {
+ {
+ .flags = IORESOURCE_MEM,
+ .start = 0x50F1A000,
+ .end = 0x50F1A037,
+ }, {
+ .flags = IORESOURCE_MEM,
+ .start = 0x50F1A038,
+ .end = 0x50F1A03B,
+ }, {
+ .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_SHAREABLE,
+ .start = IRQ_BABOON_1,
+ .end = IRQ_BABOON_1,
+ },
+};
+
+static const struct pata_platform_info mac_pata_baboon_data __initconst = {
+ .ioport_shift = 2,
+};
+
int __init mac_platform_init(void)
{
phys_addr_t swim_base = 0;
@@ -1048,6 +1093,26 @@ int __init mac_platform_init(void)
break;
}
+ /*
+* IDE device
+*/
+
+ switch (macintosh_config->ide_type) {
+ case MAC_IDE_QUADRA:
+ platform_device_register_simple("mac_ide", -1,
+ mac_ide_quadra_rsrc, ARRAY_SIZE(mac_ide_quadra_rsrc));
+ break;
+ case MAC_IDE_PB:
+ platform_device_register_simple("mac_ide", -1,
+ mac_ide_pb_rsrc, ARRAY_SIZE(mac_ide_pb_rsrc));
+ break;
+ case MAC_IDE_BABOON:
+ platform_device_register_resndata(NULL, "pata_platform", -1,
+ mac_pata_baboon_rsrc, ARRAY_SIZE(mac_pata_baboon_rsrc),
+ _pata_baboon_data, sizeof(mac_pata_baboon_data));
+ break;
+ }
+
/*
* Ethernet device
*/
diff --git a/drivers/ide/macide.c b/drivers/ide/macide.c
index 3c6bb8599303b..e16877ce719e9 100644
--- a/drivers/ide/macide.c
+++ b/drivers/ide/macide.c
@@ -18,10 +18,11 @@
#include
#include
#include
+#include
#include
-#include
-#include
+
+#define DRV_NAME "mac_ide"
#define IDE_BASE 0x50F1A000/* Base address of IDE controller */
@@ -109,42 +110,65 @@ static const char *mac_ide_name[] =
* Probe for a Macintosh IDE interface
*/
-static int __init macide_init(void)
+static int mac_ide_probe(struct platform_device *pdev)
{
- unsigned long base;
- int irq;
+ struct resource *mem, *irq;
struct ide_hw hw, *hws[] = { };
struct ide_port_info d = macide_port_info;
+ struct ide_host *host;
+ int rc;
if (!MACH_IS_MAC)
return -ENODEV;
- switch (macintosh_config->ide_type) {
- case MAC_IDE_QUADRA:
- base = IDE_BASE;
- irq = IRQ_NUBUS_F;
- break;
- case MAC_IDE_PB:
- base = IDE_BASE;
- irq = IRQ_NUBUS_C;
-
