Re: [PATCH v7 1/8] Talitos: Support for async_tx XOR offload
On Thu, Aug 30, 2012 at 7:23 AM, Geanta Neag Horia Ioan-B05471 b05...@freescale.com wrote: Besides these: 1. As Dan Williams mentioned, you should explain why you are using both spin_lock_bh and spin_lock_irqsave on the same lock. It looks like talitos_process_pending() can be called from hardirq context via talitos_error(). So spin_lock_bh is not sufficient. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v7 1/8] Talitos: Support for async_tx XOR offload
On Thu, Aug 9, 2012 at 1:20 AM, qiang@freescale.com wrote:
From: Qiang Liu qiang@freescale.com
Expose Talitos's XOR functionality to be used for RAID parity
calculation via the Async_tx layer.
Cc: Herbert Xu herb...@gondor.apana.org.au
Cc: David S. Miller da...@davemloft.net
Signed-off-by: Dipen Dudhat dipen.dud...@freescale.com
Signed-off-by: Maneesh Gupta maneesh.gu...@freescale.com
Signed-off-by: Kim Phillips kim.phill...@freescale.com
Signed-off-by: Vishnu Suresh vis...@freescale.com
Signed-off-by: Qiang Liu qiang@freescale.com
---
drivers/crypto/Kconfig |9 +
drivers/crypto/talitos.c | 413
++
drivers/crypto/talitos.h | 53 ++
3 files changed, 475 insertions(+), 0 deletions(-)
diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index be6b2ba..f0a7c29 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -222,6 +222,15 @@ config CRYPTO_DEV_TALITOS
To compile this driver as a module, choose M here: the module
will be called talitos.
+config CRYPTO_DEV_TALITOS_RAIDXOR
+ bool Talitos RAID5 XOR Calculation Offload
+ default y
No, default y. The user should explicitly enable this.
+ select DMA_ENGINE
+ depends on CRYPTO_DEV_TALITOS
+ help
+ Say 'Y' here to use the Freescale Security Engine (SEC) to
+ offload RAID XOR parity Calculation
+
Is it faster than cpu xor?
Will this engine be coordinating with another to handle memory copies?
The dma mapping code for async_tx/raid is broken when dma mapping
requests overlap or cross dma device boundaries [1].
[1]: http://marc.info/?l=linux-arm-kernelm=129407269402930w=2
+static void talitos_process_pending(struct talitos_xor_chan *xor_chan)
+{
+ struct talitos_xor_desc *desc, *_desc;
+ unsigned long flags;
+ int status;
+ struct talitos_private *priv;
+ int ch;
+
+ priv = dev_get_drvdata(xor_chan-dev);
+ ch = atomic_inc_return(priv-last_chan)
+ (priv-num_channels - 1);
Maybe a comment about why this this is duplicated from
talitos_cra_init()? It sticks out here and I had to go looking to
find out why this channel number increments on submit.
+ spin_lock_irqsave(xor_chan-desc_lock, flags);
+
+ list_for_each_entry_safe(desc, _desc, xor_chan-pending_q, node) {
+ status = talitos_submit(xor_chan-dev, ch, desc-hwdesc,
+ talitos_release_xor, desc);
+ if (status != -EINPROGRESS)
+ break;
+
+ list_del(desc-node);
+ list_add_tail(desc-node, xor_chan-in_progress_q);
+ }
+
+ spin_unlock_irqrestore(xor_chan-desc_lock, flags);
+}
+
+static void talitos_xor_run_tx_complete_actions(struct talitos_xor_desc
*desc,
+ struct talitos_xor_chan *xor_chan)
+{
+ struct device *dev = xor_chan-dev;
+ dma_addr_t dest, addr;
+ unsigned int src_cnt = desc-unmap_src_cnt;
+ unsigned int len = desc-unmap_len;
+ enum dma_ctrl_flags flags = desc-async_tx.flags;
+ struct dma_async_tx_descriptor *tx = desc-async_tx;
+
+ /* unmap dma addresses */
+ dest = desc-hwdesc.ptr[6].ptr;
+ if (likely(!(flags DMA_COMPL_SKIP_DEST_UNMAP)))
+ dma_unmap_page(dev, dest, len, DMA_BIDIRECTIONAL);
+
+ desc-idx = 6 - src_cnt;
+ if (likely(!(flags DMA_COMPL_SKIP_SRC_UNMAP))) {
+ while(desc-idx 6) {
Checkpatch says:
ERROR: space required before the open parenthesis '('
+ addr = desc-hwdesc.ptr[desc-idx++].ptr;
+ if (addr == dest)
+ continue;
+ dma_unmap_page(dev, addr, len, DMA_TO_DEVICE);
+ }
+ }
+
+ /* run dependent operations */
+ dma_run_dependencies(tx);
Here is where we run into problems if another engine accesses these
same buffers, especially on ARM v6+.
+}
+
+static void talitos_release_xor(struct device *dev, struct talitos_desc
*hwdesc,
+ void *context, int error)
+{
+ struct talitos_xor_desc *desc = context;
+ struct talitos_xor_chan *xor_chan;
+ dma_async_tx_callback callback;
+ void *callback_param;
+
+ if (unlikely(error))
+ dev_err(dev, xor operation: talitos error %d\n, error);
+
+ xor_chan = container_of(desc-async_tx.chan, struct talitos_xor_chan,
+ common);
+ spin_lock_bh(xor_chan-desc_lock);
+ if (xor_chan-completed_cookie desc-async_tx.cookie)
+ xor_chan-completed_cookie = desc-async_tx.cookie;
+
Use dma_cookie_complete().
+ callback = desc-async_tx.callback;
+ callback_param = desc-async_tx.callback_param;
+
+
Re: [PATCH v7 6/8] fsl-dma: use spin_lock_bh to instead of spin_lock_irqsave
On Thu, Aug 9, 2012 at 1:23 AM, qiang@freescale.com wrote: From: Qiang Liu qiang@freescale.com The use of spin_lock_irqsave() is a stronger locking mechanism than is required throughout the driver. The minimum locking required should be used instead. Interrupts will be turned off and context will be saved, there is needless to use irqsave. Change all instances of spin_lock_irqsave() to spin_lock_bh(). All manipulation of protected fields is done using tasklet context or weaker, which makes spin_lock_bh() the correct choice. It seems you are coordinating fsl-dma copy and talitos xor operations. It looks like fsl-dma will be called through talitos_process_pending()-dma_run_dependencies(), which is potentially called in hard irq context. This all comes back to the need to fix raid offload to manage the channels explicitly rather than the current dependency chains. -- Dan ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: Build regressions/improvements in v3.6-rc4
On Sun, Sep 2, 2012 at 11:21 AM, Geert Uytterhoeven
ge...@linux-m68k.org wrote:
JFYI, when comparing v3.6-rc4 to v3.6-rc3[3], the summaries are:
- build errors: +6/-13
6 regressions:
+ drivers/input/touchscreen/edt-ft5x06.c: error: 'struct
edt_ft5x06_ts_data' has no member named 'raw_buffer': = 846:14
powerpc-randconfig, fix reported as queued by Dmitry.
+ error: eni.c: relocation truncated to fit: R_PPC64_REL24 against
symbol `_restgpr0_25' defined in .text.save.restore section in
arch/powerpc/lib/built-in.o: = (.text+0x1ff96e4), (.text+0x1ff9438),
(.text+0x1ff8e74)
+ error: eni.c: relocation truncated to fit: R_PPC64_REL24 against
symbol `_restgpr0_28' defined in .text.save.restore section in
arch/powerpc/lib/built-in.o: = (.text+0x1ff9848)
+ error: eni.c: relocation truncated to fit: R_PPC64_REL24 against
symbol `_restgpr0_30' defined in .text.save.restore section in
arch/powerpc/lib/built-in.o: = (.text+0x1ff8ea8), (.text+0x1ff8edc)
+ error: eni.c: relocation truncated to fit: R_PPC64_REL24 against
symbol `_savegpr0_29' defined in .text.save.restore section in
arch/powerpc/lib/built-in.o: = (.text+0x1ff9cf0)
+ error: fore200e.c: relocation truncated to fit: R_PPC64_REL24
against symbol `_restgpr0_22' defined in .text.save.restore section in
arch/powerpc/lib/built-in.o: = (.text+0x1ff8608), (.text+0x1ff7e5c)
powerpc-allyesconfig
[1] http://kisskb.ellerman.id.au/kisskb/head/5381/ (all 116 configs)
[3] http://kisskb.ellerman.id.au/kisskb/head/5359/ (all 116 configs)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
-- Linus Torvalds
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] i2c-mpc: Wait for STOP to hit the bus
Tabi Timur-B04825 b04...@freescale.com wrote on 2012/09/02 04:48:01:
On Thu, Aug 30, 2012 at 5:40 AM, Joakim Tjernlund
joakim.tjernl...@transmode.se wrote:
- mpc_i2c_stop(i2c);
+ mpc_i2c_stop(i2c); /* Initiate STOP */
+ orig_jiffies = jiffies;
+ /* Wait until STOP is seen, allow up to 1 s */
+ while (readb(i2c-base + MPC_I2C_SR) CSR_MBB) {
+ if (time_after(jiffies, orig_jiffies + HZ)) {
+ u8 status = readb(i2c-base + MPC_I2C_SR);
+
+ dev_dbg(i2c-dev, timeout\n);
+ if ((status (CSR_MCF | CSR_MBB | CSR_RXAK)) != 0)
{
+ writeb(status ~CSR_MAL,
+ i2c-base + MPC_I2C_SR);
+ mpc_i2c_fixup(i2c);
+ }
+ return -EIO;
+ }
+ cond_resched();
+ }
Shouldn't the while-loop be inside mpc_i2c_stop() itself?
Possibly but I choosed to do it this way as there is a similar loop in the
beginning of mpc_xfer().
I figured it has better visibility if it is in the same function.
Jocke
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC v8 PATCH 04/20] memory-hotplug: offline and remove memory when removing the memory device
At 09/01/2012 04:55 AM, Andrew Morton Wrote:
On Tue, 28 Aug 2012 18:00:11 +0800
we...@cn.fujitsu.com wrote:
+int remove_memory(int nid, u64 start, u64 size)
+{
+int ret = -EBUSY;
+lock_memory_hotplug();
+/*
+ * The memory might become online by other task, even if you offine it.
+ * So we check whether the cpu has been onlined or not.
I think you meant memory, not cpu.
Yes. I will fix it.
Thanks
Wen Congyang
Actually, check whether any part of this memory range has been
onlined would be better. If that is accurate ;)
+ */
+if (!is_memblk_offline(start, size)) {
+pr_warn(memory removing [mem %#010llx-%#010llx] failed,
+because the memmory range is online\n,
+start, start + size);
+ret = -EAGAIN;
+}
+
+unlock_memory_hotplug();
+return ret;
+
+}
+EXPORT_SYMBOL_GPL(remove_memory);
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC v8 PATCH 08/20] memory-hotplug: remove /sys/firmware/memmap/X sysfs
At 09/01/2012 05:06 AM, Andrew Morton Wrote:
On Tue, 28 Aug 2012 18:00:15 +0800
we...@cn.fujitsu.com wrote:
From: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com
When (hot)adding memory into system, /sys/firmware/memmap/X/{end, start,
type}
sysfs files are created. But there is no code to remove these files. The
patch
implements the function to remove them.
Note : The code does not free firmware_map_entry since there is no way to
free
memory which is allocated by bootmem.
+#define to_memmap_entry(obj) container_of(obj, struct firmware_map_entry,
kobj)
It would be better to implement this as an inlined C function. That
has improved type safety and improved readability.
+static void release_firmware_map_entry(struct kobject *kobj)
+{
+struct firmware_map_entry *entry = to_memmap_entry(kobj);
+struct page *page;
+
+page = virt_to_page(entry);
+if (PageSlab(page) || PageCompound(page))
That PageCompound() test looks rather odd. Why is this done?
Liu Jiang and Christoph Lameter discussed how to find slab page
in this mail:
https://lkml.org/lkml/2012/7/6/333.
+kfree(entry);
+
+/* There is no way to free memory allocated from bootmem*/
+}
This function is a bit ugly - poking around in page flags to determine
whether or not the memory came from bootmem. It would be cleaner to
use a separate boolean. Although I guess we can live with it as you
have it here.
static struct kobj_type memmap_ktype = {
+.release= release_firmware_map_entry,
.sysfs_ops = memmap_attr_ops,
.default_attrs = def_attrs,
};
@@ -123,6 +139,16 @@ static int firmware_map_add_entry(u64 start, u64 end,
return 0;
}
+/**
+ * firmware_map_remove_entry() - Does the real work to remove a firmware
+ * memmap entry.
+ * @entry: removed entry.
+ **/
+static inline void firmware_map_remove_entry(struct firmware_map_entry
*entry)
+{
+list_del(entry-list);
+}
Is there no locking to protect that list?
/*
* Add memmap entry on sysfs
*/
@@ -144,6 +170,31 @@ static int add_sysfs_fw_map_entry(struct
firmware_map_entry *entry)
return 0;
}
+/*
+ * Remove memmap entry on sysfs
+ */
+static inline void remove_sysfs_fw_map_entry(struct firmware_map_entry
*entry)
+{
+kobject_put(entry-kobj);
+}
+
+/*
+ * Search memmap entry
+ */
+
+struct firmware_map_entry * __meminit
+find_firmware_map_entry(u64 start, u64 end, const char *type)
A better name would be firmware_map_find_entry(). To retain the (good)
convention that symbols exported from here all start with
firmware_map_.
OK.
+{
+struct firmware_map_entry *entry;
+
+list_for_each_entry(entry, map_entries, list)
+if ((entry-start == start) (entry-end == end)
+(!strcmp(entry-type, type)))
+return entry;
+
+return NULL;
+}
+
/**
* firmware_map_add_hotplug() - Adds a firmware mapping entry when we do
* memory hotplug.
@@ -196,6 +247,32 @@ int __init firmware_map_add_early(u64 start, u64 end,
const char *type)
return firmware_map_add_entry(start, end, type, entry);
}
+/**
+ * firmware_map_remove() - remove a firmware mapping entry
+ * @start: Start of the memory range.
+ * @end: End of the memory range.
+ * @type: Type of the memory range.
+ *
+ * removes a firmware mapping entry.
+ *
+ * Returns 0 on success, or -EINVAL if no entry.
+ **/
+int __meminit firmware_map_remove(u64 start, u64 end, const char *type)
+{
+struct firmware_map_entry *entry;
+
+entry = find_firmware_map_entry(start, end - 1, type);
+if (!entry)
+return -EINVAL;
+
+firmware_map_remove_entry(entry);
+
+/* remove the memmap entry */
+remove_sysfs_fw_map_entry(entry);
+
+return 0;
+}
Again, the lack of locking looks bad.
...
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1052,9 +1052,9 @@ int offline_memory(u64 start, u64 size)
return 0;
}
-int remove_memory(int nid, u64 start, u64 size)
+int __ref remove_memory(int nid, u64 start, u64 size)
Why was __ref added?
Hmm, firmware_map_remove() was put in meminit section, and we call it
in this function, so __ref is added here.
Thanks
Wen Congyang
{
-int ret = -EBUSY;
+int ret = 0;
lock_memory_hotplug();
/*
* The memory might become online by other task, even if you offine it.
...
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
