Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Finn,

Am 04.06.2021 um 17:55 schrieb Finn Thain:
On Fri, 4 Jun 2021, Michael Schmitz wrote:

of the Q40 branch and out of the later Amiga Gayle PCMCIA branch to the
head of the file and add a comment explaining the rationale? Too much
churn for now?


Right, it could be too much churn for a bug-fix patch.

I'll leave that for later (but add a comment in the lines inserted).


  #ifdef CONFIG_AMIGA_PCMCIA
@@ -135,9 +139,12 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
      case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
  #endif
  #ifdef CONFIG_ATARI_ROM_ISA
-    case ISA_TYPE_ENEC: return (u8 __iomem *)ENEC_ISA_IO_B(addr);
+    case ISA_TYPE_ENEC: if (addr < 1024)
+				return (u8 __iomem *)ENEC_ISA_IO_B(addr);
+			else
+				return (u8 __iomem *)(addr);
There is some trailing whitespace added here that 'git am' complains
about.

Also, I think a 'fallthrough' statement would be better than adding a new
else branch that duplicates the new default case below.

I'm still unsure whether changing the default branch for the sake of
fixing Atari behaviour is a sane idea - I was hoping for comments either
way.


You mean, what happens if a random m68k platform (other than amiga, atari
and q40) were to adopt CONFIG_ISA? I guess it would be nice if that would
'just work' but it's probably never going to be needed.

The NULL default was meant to catch incorrect use of config options related to CONFIG_ISA. My repurposing the default branch for Atari's needs (no translation for IDE) defeats that. But the chance that we run into such incorrect use in the future are pretty slim indeed.

Still, my patch changes behaviour in existing drivers. We're quite certain it does not matter, but is that good enough to be accepted?


But if it's changed, you are correct that having the control flow fall
through instead of taking a redundant else branch is better.

Essentially, changing that hunk to

 #ifdef CONFIG_ATARI_ROM_ISA
-    case ISA_TYPE_ENEC: return (u8 __iomem *)ENEC_ISA_IO_B(addr);
+    case ISA_TYPE_ENEC: if (addr < 1024)
+				return (u8 __iomem *)ENEC_ISA_IO_B(addr);

here (and elsewhere below)?


I worry about the static checkers that look for missing 'fallthrough' and

Never ran into 'fallthrough' except as comment annotation, but I now see that really is a thing these days. Amazing.

'break' statements. So I was thinking of something like this:

static inline u8 __iomem *isa_itb(unsigned long addr)
{
  switch(ISA_TYPE)
    {
#ifdef CONFIG_Q40
    case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
#endif
#ifdef CONFIG_AMIGA_PCMCIA
    case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
#endif
#ifdef CONFIG_ATARI_ROM_ISA
    case ISA_TYPE_ENEC:
        if (addr < 1024)
            return (u8 __iomem *)ENEC_ISA_IO_B(addr);
        fallthrough;
#endif
    default: return (u8 __iomem *)(addr);
    }
}

This one makes it easy to eventually add another ISA_TYPE_ATARI case before the default case (which could then revert to NULL if desired).



Alternatively you could just move the default out of the switch:

static inline u8 __iomem *isa_itb(unsigned long addr)
{
  switch(ISA_TYPE)
    {
#ifdef CONFIG_Q40
    case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
#endif
#ifdef CONFIG_AMIGA_PCMCIA
    case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
#endif
#ifdef CONFIG_ATARI_ROM_ISA
    case ISA_TYPE_ENEC:
        if (addr < 1024)
            return (u8 __iomem *)ENEC_ISA_IO_B(addr);
        break;
#endif
    }
    return (u8 __iomem *)(addr);
}


The latter is probably the more flexible style because 'break' doesn't
care about the order of case labels.

Yes, but it rules out reverting the default case to NULL.

On balance, I'll go with the fallback (and will annotate that other Atari drivers fall back to the clause below on purpose).

Cheers,

	Michael



[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux