Re: Uninitialized field and duplicated function in kauai.c ?

[Michael Lorenz <macallan%netbsd.org@localhost>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello,

On Jun 30, 2008, at 06:12, Marco Trillo wrote:

> Reading the arch/macppc/dev/kauai.c driver I found something strange
> regarding the 'sc_dmacmd' field for the buffer of DBDMA commands:
>
> It's defined as a pointer in kauai_softc (line numbers are for rev.  
> 1.24):
>
>     61  struct kauai_softc {
> [...]
>     68          dbdma_command_t *sc_dmacmd;
> [...]
>     74  };
>
>  However, such filed is nowhere initialized to point to a valid
> allocated buffer. kauai_attach() just tests if the buffer is 16-byte
> aligned as required by DBDMA, but as it's not initialized, it points
> to address 0 so the error should never be triggered:
>
>    106  void
>    107  kauai_attach(device_t parent, device_t self, void *aux)
>    108  {
> [...]
>    119  #ifdef DIAGNOSTIC
>    120          if ((vaddr_t)sc->sc_dmacmd & 0x0f) {
>    121                  aprint_error(": bad dbdma alignment\n");
>    122                  return;
>    123          }
>    124  #endif
>
> Then this uninitialized pointer is used by kauai_dma_init() :
>
>    286  int
>    287  kauai_dma_init(void *v, int channel, int drive, void *databuf,
>    288          size_t datalen, int flags)
>    289  {
>    290          struct kauai_softc *sc = v;
>    291          dbdma_command_t *cmdp = sc->sc_dmacmd;
> [...]
>    310                          DBDMA_BUILD(cmdp, cmd, 0, rest,  
> vtophys(va),
>
>
> It looks like it was meant to be defined as an array in the softc or
> allocated with the dbdma_alloc() function (which is what wdc_obio.c
> does), which in addition also makes sure the buffer is 16-byte
> aligned.

Hmm, you seem to be right. Makes me wonder why it works.

> There is also a private function defined in kauai.c, getnodebypci(),
> which is line-for-line identical to the pcidev_to_ofdev() function
> defined in arch/powerpc/oea/ofw_autoconf.c (in 4.0 it's on
> arch/macppc/macppc/autoconf.c).
>
>      83  static int getnodebypci(pci_chipset_tag_t, pcitag_t);
> [...]
>    126          node = getnodebypci(pa->pa_pc, pa->pa_tag);
> [...]
>    357  /*
>    358   * Find OF-device corresponding to the PCI device.
>    359   */
>    360  int
>    361  getnodebypci(pci_chipset_tag_t pc, pcitag_t tag)
> [...]

That should be replaced.

have fun
Michael

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)

iQEVAwUBSGrZdcpnzkX8Yg2nAQJFjwf8CWr/zRZDrzr5rkENHM5e6k4aa8FBLtne
h8m7gYA3iwhL7mXRvPJC1IFOp6bFudjx04rrQ7PWRrrvvpCYFBO+tnT3yAKrlAZq
kgcIQMW7JRhV4kAbR+aQ6vXtrpG6yoZ0jTGdQTJn/SjA4D2RMGpJKCu+Iql+Qz2d
3UHJBk3TZvqNWzchxEK8B3w5EcaKA/rX22h+chTijg370ZOj1SGIquAnU4IXrSka
5z/feuSIXMfd0Bf5LuG5BBdP9ZI0tsCXP73s7d0LdOHprbKH0+N4xVSHBL6h/AEI
ahP0ukvrfGbaciSss7o2meyNertNFm71Q/WKJiEI4Aly/LB/EdaP1Q==
=/vxg
-----END PGP SIGNATURE-----