linux-kernel - Re: [PATCH v3 2/2] Staging: octeon-usb: Changes macro CVMX_WAIT_FOR

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: <20180729192703.GA14390@darkstar.musicnaut.iki.fi>
Date:   Sun, 29 Jul 2018 22:27:03 +0300
From:   Aaro Koskinen <aaro.koskinen@....fi>
To:     Georgios Tsotsos <tsotsos@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        James Hogan <jhogan@...nel.org>, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] Staging: octeon-usb: Changes macro
 CVMX_WAIT_FOR_FIELD32 to function call.

Hi,

On Sun, Jul 29, 2018 at 02:40:35PM +0300, Georgios Tsotsos wrote:
> +/**
> + * Loop through register until txfflsh or rxfflsh become zero.
> + *
> + * @usb:           USB block
> + * @address:       64bit address to read
> + * @timeout_usec:  Timeout
> + * @fflsh_type:    Indicates fflsh type, 0 for txfflsh, 1 for rxfflsh
> + *
> + */
> +static int cvmx_wait_for_field32(struct octeon_hcd *usb, u64 address,
> +				 u64 timeout_usec, int fflsh_type)

You should change the name of the function, as it's no longer generic
"wait for any field" to reach specified condition.

The "address" should be calculated from "usb" inside the function.

The timeout is always 100, you could just omit it.

Nobody is ever checking the result, so it's also redundant...

[...]

> +{
> +	int result;
> +	u64 done = cvmx_get_cycle() + timeout_usec *
> +		   (u64)octeon_get_clock_rate / 1000000;
> +
> +	union cvmx_usbcx_grstctl c;
> +
> +	while (1) {
> +		c.u32 = cvmx_usb_read_csr32(usb, address);
> +		if (fflsh_type == 0 && c.s.txfflsh == 0) {
> +			result = 0;
> +			break;
> +		} else if (fflsh_type == 1 && c.s.rxfflsh == 0) {
> +			result = 0;
> +			break;
> +		} else if (cvmx_get_cycle() > done) {
> +			result = -1;
> +			break;
> +		}
> +
> +		__delay(100);
> +	}
> +	return result;
> +}
> +
>  static void cvmx_fifo_setup(struct octeon_hcd *usb)
>  {
>  	union cvmx_usbcx_ghwcfg3 usbcx_ghwcfg3;
> @@ -635,11 +648,9 @@ static void cvmx_fifo_setup(struct octeon_hcd *usb)
>  	/* Flush all FIFOs */
>  	USB_SET_FIELD32(address, cvmx_usbcx_grstctl, txfnum, 0x10);
>  	USB_SET_FIELD32(address, cvmx_usbcx_grstctl, txfflsh, 1);
> -	CVMX_WAIT_FOR_FIELD32(address, cvmx_usbcx_grstctl,
> -			      c.s.txfflsh == 0, 100);
> +	cvmx_wait_for_field32(usb, address, 0, 100);

How did you test this patch? The parameters are in wrong order, you are
specifying 0 timeout.

>  	USB_SET_FIELD32(address, cvmx_usbcx_grstctl, rxfflsh, 1);
> -	CVMX_WAIT_FOR_FIELD32(address, cvmx_usbcx_grstctl,
> -			      c.s.rxfflsh == 0, 100);
> +	cvmx_wait_for_field32(usb, address, 1, 100);

Also wrong order here.

A.