linux-kernel - Re: [PATCH 10/28] ata: separate PATA timings code from libata-core.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200129172323.GF12616@infradead.org>
Date:   Wed, 29 Jan 2020 09:23:23 -0800
From:   Christoph Hellwig <hch@...radead.org>
To:     Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Cc:     Jens Axboe <axboe@...nel.dk>,
        Michael Schmitz <schmitzmic@...il.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 10/28] ata: separate PATA timings code from libata-core.c

> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *  libata-pata-timings.c - helper library for PATA timings

Please remove the file name from the top of the file header.

> +static void ata_timing_quantize(const struct ata_timing *t, struct ata_timing *q, int T, int UT)
> +{

> +void ata_timing_merge(const struct ata_timing *a, const struct ata_timing *b,
> +		      struct ata_timing *m, unsigned int what)
> +{

Please fix the overly long lines while you're at it.

> +	if (what & ATA_TIMING_SETUP  ) m->setup   = max(a->setup,   b->setup);
> +	if (what & ATA_TIMING_ACT8B  ) m->act8b   = max(a->act8b,   b->act8b);
> +	if (what & ATA_TIMING_REC8B  ) m->rec8b   = max(a->rec8b,   b->rec8b);
> +	if (what & ATA_TIMING_CYC8B  ) m->cyc8b   = max(a->cyc8b,   b->cyc8b);
> +	if (what & ATA_TIMING_ACTIVE ) m->active  = max(a->active,  b->active);
> +	if (what & ATA_TIMING_RECOVER) m->recover = max(a->recover, b->recover);
> +	if (what & ATA_TIMING_DMACK_HOLD) m->dmack_hold = max(a->dmack_hold, b->dmack_hold);
> +	if (what & ATA_TIMING_CYCLE  ) m->cycle   = max(a->cycle,   b->cycle);
> +	if (what & ATA_TIMING_UDMA   ) m->udma    = max(a->udma,    b->udma);

and this very strange coding style.

> +	if (!(s = ata_timing_find_mode(speed)))
> +		return -EINVAL;

This should be:

	s = ata_timing_find_mode(speed);
	if (!s)
		return -EINVAL;

> +	/* In a few cases quantisation may produce enough errors to
> +	   leave t->cycle too low for the sum of active and recovery
> +	   if so we must correct this */

.. non-standard comment style here.

> +#ifdef CONFIG_ATA_ACPI
>  extern u8 ata_timing_cycle2mode(unsigned int xfer_shift, int cycle);
> +#endif

I don't think we need this ifdef - unused prototypes are completely
harmless.