Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752753AbbH0Pcy (ORCPT ); Thu, 27 Aug 2015 11:32:54 -0400 Received: from mezzanine.sirena.org.uk ([106.187.55.193]:38069 "EHLO mezzanine.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752710AbbH0Pcw (ORCPT ); Thu, 27 Aug 2015 11:32:52 -0400 Date: Thu, 27 Aug 2015 16:32:33 +0100 From: Mark Brown To: Qais Yousef Cc: alsa-devel@alsa-project.org, Liam Girdwood , Jaroslav Kysela , Takashi Iwai , linux-kernel@vger.kernel.org Message-ID: <20150827153233.GD5313@sirena.org.uk> References: <1440419959-14315-1-git-send-email-qais.yousef@imgtec.com> <1440419959-14315-4-git-send-email-qais.yousef@imgtec.com> <20150826183700.GD28760@sirena.org.uk> <55DEFF77.8070309@imgtec.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="yudcn1FV7Hsu/q59" Content-Disposition: inline In-Reply-To: <55DEFF77.8070309@imgtec.com> X-Cookie: Short people get rained on last. User-Agent: Mutt/1.5.23 (2014-03-12) X-SA-Exim-Connect-IP: 94.175.94.161 X-SA-Exim-Mail-From: broonie@sirena.org.uk Subject: Re: [PATCH 03/10] ALSA: add AXD Audio Processing IP alsa driver X-SA-Exim-Version: 4.2.1 (built Mon, 26 Dec 2011 16:24:06 +0000) X-SA-Exim-Scanned: Yes (on mezzanine.sirena.org.uk) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5881 Lines: 156 --yudcn1FV7Hsu/q59 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Aug 27, 2015 at 01:15:51PM +0100, Qais Yousef wrote: > On 08/26/2015 07:37 PM, Mark Brown wrote: > >On Mon, Aug 24, 2015 at 01:39:12PM +0100, Qais Yousef wrote: > >>+#define AXD_INPUT_DESCRIPTORS 10 > >>+struct axd_input { > >>+ struct axd_buffer_desc descriptors[AXD_INPUT_DESCRIPTORS]; > >>+}; > >Where do these numbers come from? Are they hardware limits or something > >else? > These numbers are what the firmware designed to work with. We had to set a > limit and we sought 10 to be a good one for our purposes. We don't expect to > need to change this number. So we have hard coded numbers in the firmware that we need in the driver but we can't read those numbers back from the firmware. That's sad. > >>+#define AXD_BASE_VADDR 0xD0000000 > >This sounds like something that is going to be platform dependant, > >should this be supplied from board configuration? > I don't expect this to change. Can we add the configuration later if we hit > the need to change it? It should be trivial to make things configurable shouldn't it? > >>+ if (!*offp) { > >>+ unsigned int flags = axd_platform_lock(); > >>+ unsigned int log_offset = ioread32(log_addr); > >>+ unsigned int log_wrapped = ioread32(log_addr + 8); > >>+ char __iomem *log_buff = (char __iomem *)(log_addr + 12); > >>+ > >>+ /* new read from beginning, fill up our internal buffer */ > >>+ if (!log_wrapped) { > >>+ memcpy_fromio(axd->log_rbuf, log_buff, log_offset); > >>+ axd->log_rbuf_rem = log_offset; > >>+ } else { > >>+ char __iomem *pos = log_buff + log_offset; > >>+ unsigned int rem = log_size - log_offset; > >>+ > >>+ memcpy_fromio(axd->log_rbuf, pos, rem); > >>+ memcpy_fromio(axd->log_rbuf + rem, log_buff, log_offset); > >>+ axd->log_rbuf_rem = log_size; > >>+ } > >>+ axd_platform_unlock(flags); > >I didn't see the lock being taken? > The lock is the first line in the block (unsigned int flags = > axd_platform_lock()). I'll tidy it up to make it more readable. It's very bad practice to bury lock taking in with the variable declaration. > >>+#ifdef CONFIG_CRYPTO_LZO > >>+#include > >This include should be with all the other includes, not down here. > Was trying to reduce the ifdefery. Will fix. You don't need any ifdefs for the include, you can just include the header. > >>+{ > >>+ dev_err(axd->dev, "The firmware must be lzo decompressed first, compile driver again with CONFIG_CRYPTO_LZO enabled in kernel or do the decompression in user space.\n"); > >Please split this up into a few prints for wrapping, similarly in > >several other places. > OK. I thought the convention for strings to leave them as is to allow > grepping. I'll fix it. You should keep strings that are displayed as a single string together but if you are splitting something in the output then that split won't hurt grepping in the source. > >>+ * We copy through the cache, fw will do the necessary cache > >>+ * flushes and syncing at startup. > >>+ * Copying from uncached makes it more difficult for the > >>+ * firmware to keep the caches coherent with memory when it sets > >>+ * tlbs and start running. > >>+ */ > >>+ memcpy_toio((void *)cached_fw_base, fw->data, fw->size); > >Why the cast here? I'm also not seeing where we handled the copying to > >I/O in the decompression case? > I couldn't avoid the cast. If cached_fw_base is 'void *' I'll get a warning > when initialising cached_fw_base from CAC_ADDR(). Why do you get a warning from that? Perhaps the warnings are trying to tell us something... > Good point. When decompressing crypto_comp_decompress() will write directly > to the memory. It is safe but it doesn't go through the correct API. Not > sure what I can do here. Uncompress to a buffer then write that buffer to the final destination? > >>+ dev_info(axd->dev, "Loading firmware at 0x%p ...\n", axd->fw_base_m); > >This should be _dbg() at most, otherwise it's going to get noisy. > >>+ t0_new_pc = (unsigned long) axd->fw_base_m + (t0_new_pc - AXD_BASE_VADDR); > >Those casts look fishy... > I am happy to try something else. axd->fw_base_m is of type void * __iomem > but we want to do some arithmetic on it. > Is there a better way to do it? Pointer arithmetic or converting it to a number? > >>+ /* wake up any task sleeping on command response */ > >>+ wake_up(&axd->cmd.wait); > >>+ /* give chance to user land tasks to react to the crash */ > >>+ ssleep(2); > >This looks horribly racy, I'd expect us to be trashing and/or killing > >off any active work and resources here. > OK. I was trying to play nicely by giving the chance to userland to repond > to -ERESTART which would be sent from aborting any pending reads/writes. > Are you suggesting to send SIGKILL using force_sig()? No, I'm suggesting tearing down the kernel side of any work and kicking errors back to userspace if it continues to interact with anything that was ongoing. --yudcn1FV7Hsu/q59 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJV3y2QAAoJECTWi3JdVIfQB6AH/2lFWoeoy5aS2xZpIK508WRE EnFrxd2mh0wWgyPyQdyEgX9ECrH4E4pMXYf1HzXglljtqMs3MA+ts1Y9YUlK6XCD 3K4wrJQTtxHtTqvPq7XexBxoO4B3Q7DHdmJnSL8yWj54K67wlxYhfnhZ6sqzdePS FtrSRDGlS0DD8ms3ZLtrr1ILcQGl2+OnITsJLygxZz/nyYXvmCsv0eaibEnalWsm ttAbnri/3jhs16Smw9z+Epv7SjJsL3GRGCiqK6PM8u1Zk1omayl5vuN2GA/pNQ4Q soFfdhcndu+O/y/dQiqq/k3gS0L4XqsZKZmPF164oEe4X7LmcbMDPHdlb7dQXrA= =GzA7 -----END PGP SIGNATURE----- --yudcn1FV7Hsu/q59-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/