linux-kernel - Re: [PATCH 3/3] ipc: Added support for IPC interrupt mode

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: <20131114134842.7225c7f8@alan.etchedpixels.co.uk>
Date:	Thu, 14 Nov 2013 13:48:42 +0000
From:	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>
To:	David Cohen <david.a.cohen@...ux.intel.com>
Cc:	matthew.garrett@...ula.com, platform-driver-x86@...r.kernel.org,
	linux-kernel@...r.kernel.org, eric.ernst@...ux.intel.com,
	Kuppuswamy Sathyanarayanan 
	<sathyanarayanan.kuppuswamy@...ux.intel.com>
Subject: Re: [PATCH 3/3] ipc: Added support for IPC interrupt mode

O> +	prompt "IPC access mode"
> +	depends on INTEL_SCU_IPC
> +	default INTEL_SCU_IPC_INTR_MODE
> +	---help---
> +	Select the desired access mode for IPC call.

This seems to depend at runtime on the platform so ifdefs seem
inappropriate.

>  static inline void ipc_command(u32 cmd) /* Send ipc command */
>  {
> +#ifdef CONFIG_INTEL_SCU_IPC_INTR_MODE
> +	INIT_COMPLETION(ipcdev.cmd_complete);
> +	writel(cmd | IPC_IOC, ipcdev.ipc_base);
> +#else
>  	writel(cmd, ipcdev.ipc_base);
> +#endif
>  }

If this is platform specific then add an IRQ to your platform data and
check for it then set an irq field in your scu objects and check at
runtime. If it depends upon the command and/or user then pass irq as a
parameter.

>  
>  /*
> @@ -194,6 +203,37 @@ static inline int busy_loop(void) /* Wait till scu status is busy */
>  	return 0;
>  }
>  
> +#ifdef CONFIG_INTEL_SCU_IPC_INTR_MODE
> +/* Wait till ipc ioc interrupt is received or timeout in 3 HZ */
> +static inline int ipc_wait_for_interrupt(void)
> +{
> +	int status;
> +	int ret = 0;
> +
> +	if (!wait_for_completion_timeout(&ipcdev.cmd_complete, 3 * HZ)) {
> +		ret = -ETIMEDOUT;
> +		goto end;
> +	}
> +
> +	status = ipc_read_status();
> +
> +	if ((status >> 1) & 1)
> +		ret = -EIO;
> +
> +end:
> +	return ret;

What happens on a timeout if the command then completes. Will it not
potentially produce a bogus completion on the next command just being
issued in some cases. Also it should probably be logged ?

So I think

1. Pass the informatio upon whether IRQ mode should be used in the
platform information and remove all the ifdeffery
2. Log timeouts
3. Explain what happens on a timeout that allows you to issue further
commands without a race - eg do you need to do any kind of reset or will
the next command issue stall sufficiently etc ?

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/