From: "Allen Hubbe" <Allen.Hubbe@dell.com> To: "'Serge Semin'" <fancer.lancer@gmail.com>, <jdmason@kudzu.us>, <dave.jiang@intel.com>, <Xiangliang.Yu@amd.com> Cc: <Sergey.Semin@t-platforms.ru>, <linux-ntb@googlegroups.com>, <linux-kernel@vger.kernel.org> Subject: RE: [PATCH v2] NTB: Add IDT 89HPESxNTx PCIe-switches support Date: Tue, 21 Feb 2017 17:42:37 -0500 [thread overview] Message-ID: <000001d28c93$cea16310$6be42930$@dell.com> (raw) In-Reply-To: <1487626394-16469-1-git-send-email-fancer.lancer@gmail.com> From: Serge Semin > +/* > + * idt_nt_write() - PCI configuration space registers write method > + * @ndev: IDT NTB hardware driver descriptor > + * @reg: Register to write data to > + * @data: Value to write to the register > + * > + * WARNING! IDT PCIe-switch registers are all Little endian. So corresponding > + * writel operations must have embedded endiannes conversion. If local > + * platform doesn't have it, the driver won't properly work. > + */ > +static void idt_nt_write(struct idt_ntb_dev *ndev, > + const unsigned int reg, const u32 data) > +{ > + /* > + * It's obvious bug to request a register exceeding the maximum possible > + * value as well as to have it unaligned. > + */ > + WARN_ON(reg > IDT_REG_PCI_MAX || !IS_ALIGNED(reg, IDT_REG_ALIGN)); If we perform the write anyway, I guess the effect of the write is unknown? What about: if (WARN_ON(stuff)) return; > +/* > + * idt_reg_set_bits() - set bits of a passed register > + * @ndev: IDT NTB hardware driver descriptor > + * @reg: Register to change bits of > + * @valid_mask: Mask of valid bits > + * @set_bits: Bitmask to set > + * > + * Helper method to check whether a passed bitfield is valid and set > + * corresponding bits of a register. > + * > + * Return: zero on success, negative error on invalid bitmask. > + */ > +static inline int idt_reg_set_bits(struct idt_ntb_dev *ndev, unsigned int reg, > + u64 valid_mask, u64 set_bits) > +{ > + u32 data; > + > + if (set_bits & ~(u64)valid_mask) > + return -EINVAL; > + > + data = idt_nt_read(ndev, reg) | (u32)set_bits; > + idt_nt_write(ndev, IDT_NT_INDBELLMSK, data); Following this function call via itd_ntb_db_set_mask(), it does not appear that the register update is atomic here. Two threads could read the same old register value and modify it differently, and the second write back to the register would clobber the first. In the ntb_hw_intel driver there is a similar setting bits of the doorbell mask, and clearing bits. Instead of reading the register, modifying it, and writing it back, the current value of the register is stored in memory. With a spin lock held, the value is updated in memory and then written to the register. That makes the update atomic, because the spin lock is held through the update and issuing the write, and it saves a trip to read the register. > +/* > + * idt_get_mw_type() - get memory window size Doc doesn't match the function name. > + * @mw_type: Memory window type > + * > + * Return: number of memory windows corresponding to the type This is more like a "count" than a "size". > + */ > +static inline unsigned char idt_get_mw_size(enum idt_mw_type mw_type) > +{ > +/* > + * idt_ntb_db_set_mask() - set bits in the local doorbell mask > + * (NTB API callback) > + * @ntb: NTB device context. > + * @db_bits: Doorbell mask bits to set. > + * > + * The inbound doorbell register mask value must be read, then OR'ed with > + * passed field and only then set back. > + * > + * Return: zero on success, negative error if invalid argument passed. > + */ > +static int idt_ntb_db_set_mask(struct ntb_dev *ntb, u64 db_bits) > +{ > + struct idt_ntb_dev *ndev = to_ndev_ntb(ntb); > + > + return idt_reg_set_bits(ndev, IDT_NT_INDBELLMSK, IDT_DBELL_MASK, > + db_bits); As noted above, this does not appear to be atomic.
WARNING: multiple messages have this Message-ID (diff)
From: "Allen Hubbe" <Allen.Hubbe@dell.com> To: 'Serge Semin' <fancer.lancer@gmail.com>, jdmason@kudzu.us, dave.jiang@intel.com, Xiangliang.Yu@amd.com Cc: Sergey.Semin@t-platforms.ru, linux-ntb@googlegroups.com, linux-kernel@vger.kernel.org Subject: RE: [PATCH v2] NTB: Add IDT 89HPESxNTx PCIe-switches support Date: Tue, 21 Feb 2017 17:42:37 -0500 [thread overview] Message-ID: <000001d28c93$cea16310$6be42930$@dell.com> (raw) In-Reply-To: <1487626394-16469-1-git-send-email-fancer.lancer@gmail.com> From: Serge Semin > +/* > + * idt_nt_write() - PCI configuration space registers write method > + * @ndev: IDT NTB hardware driver descriptor > + * @reg: Register to write data to > + * @data: Value to write to the register > + * > + * WARNING! IDT PCIe-switch registers are all Little endian. So corresponding > + * writel operations must have embedded endiannes conversion. If local > + * platform doesn't have it, the driver won't properly work. > + */ > +static void idt_nt_write(struct idt_ntb_dev *ndev, > + const unsigned int reg, const u32 data) > +{ > + /* > + * It's obvious bug to request a register exceeding the maximum possible > + * value as well as to have it unaligned. > + */ > + WARN_ON(reg > IDT_REG_PCI_MAX || !IS_ALIGNED(reg, IDT_REG_ALIGN)); If we perform the write anyway, I guess the effect of the write is unknown? What about: if (WARN_ON(stuff)) return; > +/* > + * idt_reg_set_bits() - set bits of a passed register > + * @ndev: IDT NTB hardware driver descriptor > + * @reg: Register to change bits of > + * @valid_mask: Mask of valid bits > + * @set_bits: Bitmask to set > + * > + * Helper method to check whether a passed bitfield is valid and set > + * corresponding bits of a register. > + * > + * Return: zero on success, negative error on invalid bitmask. > + */ > +static inline int idt_reg_set_bits(struct idt_ntb_dev *ndev, unsigned int reg, > + u64 valid_mask, u64 set_bits) > +{ > + u32 data; > + > + if (set_bits & ~(u64)valid_mask) > + return -EINVAL; > + > + data = idt_nt_read(ndev, reg) | (u32)set_bits; > + idt_nt_write(ndev, IDT_NT_INDBELLMSK, data); Following this function call via itd_ntb_db_set_mask(), it does not appear that the register update is atomic here. Two threads could read the same old register value and modify it differently, and the second write back to the register would clobber the first. In the ntb_hw_intel driver there is a similar setting bits of the doorbell mask, and clearing bits. Instead of reading the register, modifying it, and writing it back, the current value of the register is stored in memory. With a spin lock held, the value is updated in memory and then written to the register. That makes the update atomic, because the spin lock is held through the update and issuing the write, and it saves a trip to read the register. > +/* > + * idt_get_mw_type() - get memory window size Doc doesn't match the function name. > + * @mw_type: Memory window type > + * > + * Return: number of memory windows corresponding to the type This is more like a "count" than a "size". > + */ > +static inline unsigned char idt_get_mw_size(enum idt_mw_type mw_type) > +{ > +/* > + * idt_ntb_db_set_mask() - set bits in the local doorbell mask > + * (NTB API callback) > + * @ntb: NTB device context. > + * @db_bits: Doorbell mask bits to set. > + * > + * The inbound doorbell register mask value must be read, then OR'ed with > + * passed field and only then set back. > + * > + * Return: zero on success, negative error if invalid argument passed. > + */ > +static int idt_ntb_db_set_mask(struct ntb_dev *ntb, u64 db_bits) > +{ > + struct idt_ntb_dev *ndev = to_ndev_ntb(ntb); > + > + return idt_reg_set_bits(ndev, IDT_NT_INDBELLMSK, IDT_DBELL_MASK, > + db_bits); As noted above, this does not appear to be atomic.
next prev parent reply other threads:[~2017-02-21 22:52 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-02-01 21:37 [PATCH] NTB: Add IDT 89HPESxNTx PCIe-switches support Serge Semin 2017-02-02 18:10 ` Allen Hubbe 2017-02-02 18:10 ` Allen Hubbe 2017-02-20 21:33 ` [PATCH v2] " Serge Semin 2017-02-21 22:42 ` Allen Hubbe [this message] 2017-02-21 22:42 ` Allen Hubbe 2017-02-22 11:01 ` Serge Semin 2017-02-24 10:49 ` [PATCH v3] " Serge Semin 2017-02-24 16:01 ` Allen Hubbe 2017-02-24 16:01 ` Allen Hubbe 2017-02-27 9:22 ` [PATCH v4] " Serge Semin 2017-03-01 16:30 ` Jon Mason 2017-03-03 6:38 ` lsgunthorpe 2017-03-07 1:57 ` Serge Semin 2017-03-07 3:27 ` Logan Gunthorpe 2017-03-07 15:09 ` Serge Semin 2017-03-07 17:00 ` Logan Gunthorpe 2017-03-07 2:02 ` [PATCH v5] " Serge Semin 2017-03-08 18:01 ` Jon Mason 2017-03-08 20:29 ` [PATCH v6] " Serge Semin 2017-04-12 12:44 ` [PATCH v7] " Serge Semin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='000001d28c93$cea16310$6be42930$@dell.com' \ --to=allen.hubbe@dell.com \ --cc=Sergey.Semin@t-platforms.ru \ --cc=Xiangliang.Yu@amd.com \ --cc=dave.jiang@intel.com \ --cc=fancer.lancer@gmail.com \ --cc=jdmason@kudzu.us \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-ntb@googlegroups.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.