Re: [PATCH 08/22] crypto: chcr: Make use of the new sg_map helper function

[Harsh Jain]

On Fri, Apr 14, 2017 at 3:35 AM, Logan Gunthorpe  wrote:
> The get_page in this area looks *highly* suspect due to there being no
> corresponding put_page. However, I've left that as is to avoid breaking
> things.
chcr driver will post the request to LLD driver cxgb4 and put_page is
implemented there. it will no harm. Any how
we have removed the below code from driver.

http://www.mail-archive.com/linux-crypto@vger.kernel.org/msg24561.html

After this merge we can ignore your patch. Thanks

>
> I've also removed the KMAP_ATOMIC_ARGS check as it appears to be dead
> code that dates back to when it was first committed...


>
> Signed-off-by: Logan Gunthorpe 
> ---
>  drivers/crypto/chelsio/chcr_algo.c | 28 +++-
>  1 file changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/crypto/chelsio/chcr_algo.c 
> b/drivers/crypto/chelsio/chcr_algo.c
> index 41bc7f4..a993d1d 100644
> --- a/drivers/crypto/chelsio/chcr_algo.c
> +++ b/drivers/crypto/chelsio/chcr_algo.c
> @@ -1489,22 +1489,21 @@ static struct sk_buff *create_authenc_wr(struct 
> aead_request *req,
> return ERR_PTR(-EINVAL);
>  }
>
> -static void aes_gcm_empty_pld_pad(struct scatterlist *sg,
> - unsigned short offset)
> +static int aes_gcm_empty_pld_pad(struct scatterlist *sg,
> +unsigned short offset)
>  {
> -   struct page *spage;
> unsigned char *addr;
>
> -   spage = sg_page(sg);
> -   get_page(spage); /* so that it is not freed by NIC */
> -#ifdef KMAP_ATOMIC_ARGS
> -   addr = kmap_atomic(spage, KM_SOFTIRQ0);
> -#else
> -   addr = kmap_atomic(spage);
> -#endif
> -   memset(addr + sg->offset, 0, offset + 1);
> +   get_page(sg_page(sg)); /* so that it is not freed by NIC */
> +
> +   addr = sg_map(sg, SG_KMAP_ATOMIC);
> +   if (IS_ERR(addr))
> +   return PTR_ERR(addr);
> +
> +   memset(addr, 0, offset + 1);
> +   sg_unmap(sg, addr, SG_KMAP_ATOMIC);
>
> -   kunmap_atomic(addr);
> +   return 0;
>  }
>
>  static int set_msg_len(u8 *block, unsigned int msglen, int csize)
> @@ -1940,7 +1939,10 @@ static struct sk_buff *create_gcm_wr(struct 
> aead_request *req,
> if (req->cryptlen) {
> write_sg_to_skb(skb, , src, req->cryptlen);
> } else {
> -   aes_gcm_empty_pld_pad(req->dst, authsize - 1);
> +   err = aes_gcm_empty_pld_pad(req->dst, authsize - 1);
> +   if (err)
> +   goto dstmap_fail;
> +
> write_sg_to_skb(skb, , reqctx->dst, crypt_len);
>
> }
> --
> 2.1.4
>

[PATCH 1/7] PCI: export pcie_flr

[Christoph Hellwig]

Currently we opencode the FLR sequence in lots of place, export a core
helper instead.  We split out the probing for FLR support as all the
non-core callers already know their hardware.

Note that in the new pci_has_flr function the quirk check has been moved
before the capability check as there is no point in reading the
capability in this case.

Signed-off-by: Christoph Hellwig 
Acked-by: Bjorn Helgaas 
---
 drivers/pci/pci.c   | 39 ---
 include/linux/pci.h |  1 +
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bef14777bb30..957a11a6a840 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3773,27 +3773,41 @@ static void pci_flr_wait(struct pci_dev *dev)
 (i - 1) * 100);
 }
 
-static int pcie_flr(struct pci_dev *dev, int probe)
+/**
+ * pcie_has_flr - check if a device supports function level resets
+ * @dev:   device to check
+ *
+ * Returns true if the device advertises support for PCIe function level
+ * resets.
+ */
+static bool pcie_has_flr(struct pci_dev *dev)
 {
u32 cap;
 
-   pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
-   if (!(cap & PCI_EXP_DEVCAP_FLR))
-   return -ENOTTY;
-
if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET)
-   return -ENOTTY;
+   return false;
 
-   if (probe)
-   return 0;
+   pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
+   return cap & PCI_EXP_DEVCAP_FLR;
+}
 
+/**
+ * pcie_flr - initiate a PCIe function level reset
+ * @dev:   device to reset
+ *
+ * Initiate a function level reset on @dev.  The caller should ensure the
+ * device supports FLR before calling this function, e.g. by using the
+ * pcie_has_flr() helper.
+ */
+void pcie_flr(struct pci_dev *dev)
+{
if (!pci_wait_for_pending_transaction(dev))
dev_err(>dev, "timed out waiting for pending transaction; 
performing function level reset anyway\n");
 
pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
pci_flr_wait(dev);
-   return 0;
 }
+EXPORT_SYMBOL_GPL(pcie_flr);
 
 static int pci_af_flr(struct pci_dev *dev, int probe)
 {
@@ -3977,9 +3991,12 @@ static int __pci_dev_reset(struct pci_dev *dev, int 
probe)
if (rc != -ENOTTY)
goto done;
 
-   rc = pcie_flr(dev, probe);
-   if (rc != -ENOTTY)
+   if (pcie_has_flr(dev)) {
+   if (!probe)
+   pcie_flr(dev);
+   rc = 0;
goto done;
+   }
 
rc = pci_af_flr(dev, probe);
if (rc != -ENOTTY)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 20e1865233a4..60162f51227a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1054,6 +1054,7 @@ int pcie_get_mps(struct pci_dev *dev);
 int pcie_set_mps(struct pci_dev *dev, int mps);
 int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
  enum pcie_link_width *width);
+void pcie_flr(struct pci_dev *dev);
 int __pci_reset_function(struct pci_dev *dev);
 int __pci_reset_function_locked(struct pci_dev *dev);
 int pci_reset_function(struct pci_dev *dev);
-- 
2.11.0

export pcie_flr and remove copies of it in drivers V2

[Christoph Hellwig]

Hi all,

this exports the PCI layer pcie_flr helper, and removes various opencoded
copies of it.

Changes since V1:
 - rebase on top of the pci/virtualization branch
 - fixed the probe case in __pci_dev_reset
 - added ACKs from Bjorn

[PATCH 3/7] PCI: call pcie_flr from reset_chelsio_generic_dev

[Christoph Hellwig]

Instead of copy & pasting and old version of the code.

Signed-off-by: Christoph Hellwig 
Acked-by: Bjorn Helgaas 
---
 drivers/pci/quirks.c | 15 +--
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 8195ca294ee5..715ed8c08fa3 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3750,20 +3750,7 @@ static int reset_chelsio_generic_dev(struct pci_dev 
*dev, int probe)
  PCI_MSIX_FLAGS_ENABLE |
  PCI_MSIX_FLAGS_MASKALL);
 
-   /*
-* Start of pcie_flr() code sequence.  This reset code is a copy of
-* the guts of pcie_flr() because that's not an exported function.
-*/
-
-   if (!pci_wait_for_pending_transaction(dev))
-   dev_err(>dev, "transaction is not cleared; proceeding with 
reset anyway\n");
-
-   pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
-   msleep(100);
-
-   /*
-* End of pcie_flr() code sequence.
-*/
+   pcie_flr(dev);
 
/*
 * Restore the configuration information (BAR values, etc.) including
-- 
2.11.0

[PATCH 6/7] crypto: qat: use pcie_flr instead of duplicating it

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig 
---
 drivers/crypto/qat/qat_common/adf_aer.c | 15 +--
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/crypto/qat/qat_common/adf_aer.c 
b/drivers/crypto/qat/qat_common/adf_aer.c
index 2839fccdd84b..d3e25c37dc33 100644
--- a/drivers/crypto/qat/qat_common/adf_aer.c
+++ b/drivers/crypto/qat/qat_common/adf_aer.c
@@ -109,20 +109,7 @@ EXPORT_SYMBOL_GPL(adf_reset_sbr);
 
 void adf_reset_flr(struct adf_accel_dev *accel_dev)
 {
-   struct pci_dev *pdev = accel_to_pci_dev(accel_dev);
-   u16 control = 0;
-   int pos = 0;
-
-   dev_info(_DEV(accel_dev), "Function level reset\n");
-   pos = pci_pcie_cap(pdev);
-   if (!pos) {
-   dev_err(_DEV(accel_dev), "Restart device failed\n");
-   return;
-   }
-   pci_read_config_word(pdev, pos + PCI_EXP_DEVCTL, );
-   control |= PCI_EXP_DEVCTL_BCR_FLR;
-   pci_write_config_word(pdev, pos + PCI_EXP_DEVCTL, control);
-   msleep(100);
+   pcie_flr(accel_to_pci_dev(accel_dev));
 }
 EXPORT_SYMBOL_GPL(adf_reset_flr);
 
-- 
2.11.0

[PATCH 4/7] ixgbe: use pcie_flr instead of duplicating it

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig 
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 16 ++--
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c 
b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index a7a430a7be2c..543ddde5f8e2 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7112,18 +7112,6 @@ static void ixgbe_watchdog_flush_tx(struct ixgbe_adapter 
*adapter)
 }
 
 #ifdef CONFIG_PCI_IOV
-static inline void ixgbe_issue_vf_flr(struct ixgbe_adapter *adapter,
- struct pci_dev *vfdev)
-{
-   if (!pci_wait_for_pending_transaction(vfdev))
-   e_dev_warn("Issuing VFLR with pending transactions\n");
-
-   e_dev_err("Issuing VFLR for VF %s\n", pci_name(vfdev));
-   pcie_capability_set_word(vfdev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
-
-   msleep(100);
-}
-
 static void ixgbe_check_for_bad_vf(struct ixgbe_adapter *adapter)
 {
struct ixgbe_hw *hw = >hw;
@@ -7156,7 +7144,7 @@ static void ixgbe_check_for_bad_vf(struct ixgbe_adapter 
*adapter)
pci_read_config_word(vfdev, PCI_STATUS, _reg);
if (status_reg != IXGBE_FAILED_READ_CFG_WORD &&
status_reg & PCI_STATUS_REC_MASTER_ABORT)
-   ixgbe_issue_vf_flr(adapter, vfdev);
+   pcie_flr(vfdev);
}
 }
 
@@ -10244,7 +10232,7 @@ static pci_ers_result_t ixgbe_io_error_detected(struct 
pci_dev *pdev,
 * VFLR.  Just clean up the AER in that case.
 */
if (vfdev) {
-   ixgbe_issue_vf_flr(adapter, vfdev);
+   pcie_flr(vfdev);
/* Free device reference count */
pci_dev_put(vfdev);
}
-- 
2.11.0

[PATCH 7/7] liquidio: use pcie_flr instead of duplicating it

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig 
---
 drivers/net/ethernet/cavium/liquidio/lio_vf_main.c | 15 +--
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c 
b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
index 9d5e03502c76..afdbf7fa016e 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
@@ -869,20 +869,7 @@ static void octeon_pci_flr(struct octeon_device *oct)
pci_write_config_word(oct->pci_dev, PCI_COMMAND,
  PCI_COMMAND_INTX_DISABLE);
 
-   /* Wait for Transaction Pending bit clean */
-   msleep(100);
-   pcie_capability_read_word(oct->pci_dev, PCI_EXP_DEVSTA, );
-   if (status & PCI_EXP_DEVSTA_TRPND) {
-   dev_info(>pci_dev->dev, "Function reset incomplete after 
100ms, sleeping for 5 seconds\n");
-   ssleep(5);
-   pcie_capability_read_word(oct->pci_dev, PCI_EXP_DEVSTA,
- );
-   if (status & PCI_EXP_DEVSTA_TRPND)
-   dev_info(>pci_dev->dev, "Function reset still 
incomplete after 5s, reset anyway\n");
-   }
-   pcie_capability_set_word(oct->pci_dev, PCI_EXP_DEVCTL,
-PCI_EXP_DEVCTL_BCR_FLR);
-   mdelay(100);
+   pcie_flr(oct->pci_dev);
 
pci_cfg_access_unlock(oct->pci_dev);
 
-- 
2.11.0

[PATCH 5/7] IB/hfi1: use pcie_flr instead of duplicating it

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig 
---
 drivers/infiniband/hw/hfi1/chip.c |  4 ++--
 drivers/infiniband/hw/hfi1/hfi.h  |  1 -
 drivers/infiniband/hw/hfi1/pcie.c | 30 --
 3 files changed, 2 insertions(+), 33 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/chip.c 
b/drivers/infiniband/hw/hfi1/chip.c
index 121a4c920f1b..d037f72e4d96 100644
--- a/drivers/infiniband/hw/hfi1/chip.c
+++ b/drivers/infiniband/hw/hfi1/chip.c
@@ -13610,14 +13610,14 @@ static void init_chip(struct hfi1_devdata *dd)
dd_dev_info(dd, "Resetting CSRs with FLR\n");
 
/* do the FLR, the DC reset will remain */
-   hfi1_pcie_flr(dd);
+   pcie_flr(dd->pcidev);
 
/* restore command and BARs */
restore_pci_variables(dd);
 
if (is_ax(dd)) {
dd_dev_info(dd, "Resetting CSRs with FLR\n");
-   hfi1_pcie_flr(dd);
+   pcie_flr(dd->pcidev);
restore_pci_variables(dd);
}
} else {
diff --git a/drivers/infiniband/hw/hfi1/hfi.h b/drivers/infiniband/hw/hfi1/hfi.h
index 0808e3c3ba39..40d7559fa723 100644
--- a/drivers/infiniband/hw/hfi1/hfi.h
+++ b/drivers/infiniband/hw/hfi1/hfi.h
@@ -1764,7 +1764,6 @@ int hfi1_pcie_init(struct pci_dev *, const struct 
pci_device_id *);
 void hfi1_pcie_cleanup(struct pci_dev *);
 int hfi1_pcie_ddinit(struct hfi1_devdata *, struct pci_dev *);
 void hfi1_pcie_ddcleanup(struct hfi1_devdata *);
-void hfi1_pcie_flr(struct hfi1_devdata *);
 int pcie_speeds(struct hfi1_devdata *);
 void request_msix(struct hfi1_devdata *, u32 *, struct hfi1_msix_entry *);
 void hfi1_enable_intx(struct pci_dev *);
diff --git a/drivers/infiniband/hw/hfi1/pcie.c 
b/drivers/infiniband/hw/hfi1/pcie.c
index 0829fce06172..c81556e84831 100644
--- a/drivers/infiniband/hw/hfi1/pcie.c
+++ b/drivers/infiniband/hw/hfi1/pcie.c
@@ -240,36 +240,6 @@ void hfi1_pcie_ddcleanup(struct hfi1_devdata *dd)
iounmap(dd->piobase);
 }
 
-/*
- * Do a Function Level Reset (FLR) on the device.
- * Based on static function drivers/pci/pci.c:pcie_flr().
- */
-void hfi1_pcie_flr(struct hfi1_devdata *dd)
-{
-   int i;
-   u16 status;
-
-   /* no need to check for the capability - we know the device has it */
-
-   /* wait for Transaction Pending bit to clear, at most a few ms */
-   for (i = 0; i < 4; i++) {
-   if (i)
-   msleep((1 << (i - 1)) * 100);
-
-   pcie_capability_read_word(dd->pcidev, PCI_EXP_DEVSTA, );
-   if (!(status & PCI_EXP_DEVSTA_TRPND))
-   goto clear;
-   }
-
-   dd_dev_err(dd, "Transaction Pending bit is not clearing, proceeding 
with reset anyway\n");
-
-clear:
-   pcie_capability_set_word(dd->pcidev, PCI_EXP_DEVCTL,
-PCI_EXP_DEVCTL_BCR_FLR);
-   /* PCIe spec requires the function to be back within 100ms */
-   msleep(100);
-}
-
 static void msix_setup(struct hfi1_devdata *dd, int pos, u32 *msixcnt,
   struct hfi1_msix_entry *hfi1_msix_entry)
 {
-- 
2.11.0

[PATCH 2/7] PCI: call pcie_flr from reset_intel_82599_sfp_virtfn

[Christoph Hellwig]

The 82599 quirk contained an outdated copy of the FLR code.

Signed-off-by: Christoph Hellwig 
Acked-by: Bjorn Helgaas 
---
 drivers/pci/quirks.c | 16 
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 823271b13d12..8195ca294ee5 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3641,19 +3641,11 @@ static int reset_intel_82599_sfp_virtfn(struct pci_dev 
*dev, int probe)
 *
 * The 82599 supports FLR on VFs, but FLR support is reported only
 * in the PF DEVCAP (sec 9.3.10.4), not in the VF DEVCAP (sec 9.5).
-* Therefore, we can't use pcie_flr(), which checks the VF DEVCAP.
+* Thus we must call pcie_flr directly without first checking if it is
+* supported.
 */
-
-   if (probe)
-   return 0;
-
-   if (!pci_wait_for_pending_transaction(dev))
-   dev_err(>dev, "transaction is not cleared; proceeding with 
reset anyway\n");
-
-   pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
-
-   msleep(100);
-
+   if (!probe)
+   pcie_flr(dev);
return 0;
 }
 
-- 
2.11.0

Re: [PATCH 04/22] target: Make use of the new sg_map function at 16 call sites (fwd)

[Logan Gunthorpe]

Thanks Julia. I missed that and I'll fix it in my series.

Logan

On 14/04/17 09:19 AM, Julia Lawall wrote:
> It looks like >cmdr_lock should be released at line 512 if it has
> not been released otherwise.  The lock was taken at line 438.
> 
> julia
> 
> -- Forwarded message --
> Date: Fri, 14 Apr 2017 22:21:44 +0800
> From: kbuild test robot <fengguang...@intel.com>
> To: kbu...@01.org
> Cc: Julia Lawall <julia.law...@lip6.fr>
> Subject: Re: [PATCH 04/22] target: Make use of the new sg_map function at 16
> call sites
> 
> Hi Logan,
> 
> [auto build test WARNING on scsi/for-next]
> [also build test WARNING on v4.11-rc6]
> [cannot apply to next-20170413]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Logan-Gunthorpe/Introduce-common-scatterlist-map-function/20170414-142518
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
> :: branch date: 8 hours ago
> :: commit date: 8 hours ago
> 
>>> drivers/target/target_core_user.c:512:2-8: preceding lock on line 438
>drivers/target/target_core_user.c:512:2-8: preceding lock on line 471
> 
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 78082134e7afdc59d744eb8d2def5c588e89c378
> vim +512 drivers/target/target_core_user.c
> 
> 7c9e7a6f Andy Grover  2014-10-01  432 
> sizeof(struct tcmu_cmd_entry));
> 7c9e7a6f Andy Grover  2014-10-01  433 command_size = base_command_size
> 7c9e7a6f Andy Grover  2014-10-01  434 + 
> round_up(scsi_command_size(se_cmd->t_task_cdb), TCMU_OP_ALIGN_SIZE);
> 7c9e7a6f Andy Grover  2014-10-01  435
> 7c9e7a6f Andy Grover  2014-10-01  436 WARN_ON(command_size & 
> (TCMU_OP_ALIGN_SIZE-1));
> 7c9e7a6f Andy Grover  2014-10-01  437
> 7c9e7a6f Andy Grover  2014-10-01 @438 spin_lock_irq(>cmdr_lock);
> 7c9e7a6f Andy Grover  2014-10-01  439
> 7c9e7a6f Andy Grover  2014-10-01  440 mb = udev->mb_addr;
> 7c9e7a6f Andy Grover  2014-10-01  441 cmd_head = mb->cmd_head % 
> udev->cmdr_size; /* UAM */
> 26418649 Sheng Yang   2016-02-26  442 data_length = 
> se_cmd->data_length;
> 26418649 Sheng Yang   2016-02-26  443 if (se_cmd->se_cmd_flags & 
> SCF_BIDI) {
> 26418649 Sheng Yang   2016-02-26  444 
> BUG_ON(!(se_cmd->t_bidi_data_sg && se_cmd->t_bidi_data_nents));
> 26418649 Sheng Yang   2016-02-26  445 data_length += 
> se_cmd->t_bidi_data_sg->length;
> 26418649 Sheng Yang   2016-02-26  446 }
> 554617b2 Andy Grover  2016-08-25  447 if ((command_size > 
> (udev->cmdr_size / 2)) ||
> 554617b2 Andy Grover  2016-08-25  448 data_length > 
> udev->data_size) {
> 554617b2 Andy Grover  2016-08-25  449 pr_warn("TCMU: Request 
> of size %zu/%zu is too big for %u/%zu "
> 3d9b9555 Andy Grover  2016-08-25  450 "cmd ring/data 
> area\n", command_size, data_length,
> 7c9e7a6f Andy Grover  2014-10-01  451 
> udev->cmdr_size, udev->data_size);
> 554617b2 Andy Grover  2016-08-25  452 
> spin_unlock_irq(>cmdr_lock);
> 554617b2 Andy Grover  2016-08-25  453 return 
> TCM_INVALID_CDB_FIELD;
> 554617b2 Andy Grover  2016-08-25  454 }
> 7c9e7a6f Andy Grover  2014-10-01  455
> 26418649 Sheng Yang   2016-02-26  456 while 
> (!is_ring_space_avail(udev, command_size, data_length)) {
> 7c9e7a6f Andy Grover  2014-10-01  457 int ret;
> 7c9e7a6f Andy Grover  2014-10-01  458 DEFINE_WAIT(__wait);
> 7c9e7a6f Andy Grover  2014-10-01  459
> 7c9e7a6f Andy Grover  2014-10-01  460 
> prepare_to_wait(>wait_cmdr, &__wait, TASK_INTERRUPTIBLE);
> 7c9e7a6f Andy Grover  2014-10-01  461
> 7c9e7a6f Andy Grover  2014-10-01  462 pr_debug("sleeping for 
> ring space\n");
> 7c9e7a6f Andy Grover  2014-10-01  463 
> spin_unlock_irq(>cmdr_lock);
> 7c9e7a6f Andy Grover  2014-10-01  464 ret = 
> schedule_timeout(msecs_to_jiffies(TCMU_TIME_OUT));
> 7c9e7a6f Andy Grover  2014-10-01  465 
> finish_wait(>wait_cmdr, &__wait);
> 7c9e7a6f Andy Grover  2014-10-01  466 if (!ret) {
> 7c9e7a6f Andy Grover  2014-10-01  467 pr_warn("tcmu: 
> command timed out\n");
> 02eb924f Andy Grover  2016-10-06  468 

RE: [PATCH 10/22] staging: unisys: visorbus: Make use of the new sg_map helper function

[Kershner, David A]

> -Original Message-
> From: Logan Gunthorpe [mailto:log...@deltatee.com]
...
> Subject: [PATCH 10/22] staging: unisys: visorbus: Make use of the new
> sg_map helper function
> 
> Straightforward conversion to the new function.
> 
> Signed-off-by: Logan Gunthorpe 

Can you add Acked-by for this patch? 

Acked-by: David Kershner 

Tested on s-Par and no problems. 

Thanks,
David Kershner

> ---
>  drivers/staging/unisys/visorhba/visorhba_main.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c
> b/drivers/staging/unisys/visorhba/visorhba_main.c
> index 0ce92c8..2d8c8bc 100644
> --- a/drivers/staging/unisys/visorhba/visorhba_main.c
> +++ b/drivers/staging/unisys/visorhba/visorhba_main.c
> @@ -842,7 +842,6 @@ do_scsi_nolinuxstat(struct uiscmdrsp *cmdrsp, struct
> scsi_cmnd *scsicmd)
>   struct scatterlist *sg;
>   unsigned int i;
>   char *this_page;
> - char *this_page_orig;
>   int bufind = 0;
>   struct visordisk_info *vdisk;
>   struct visorhba_devdata *devdata;
> @@ -869,11 +868,14 @@ do_scsi_nolinuxstat(struct uiscmdrsp *cmdrsp,
> struct scsi_cmnd *scsicmd)
> 
>   sg = scsi_sglist(scsicmd);
>   for (i = 0; i < scsi_sg_count(scsicmd); i++) {
> - this_page_orig = kmap_atomic(sg_page(sg + i));
> - this_page = (void *)((unsigned long)this_page_orig |
> -  sg[i].offset);
> + this_page = sg_map(sg + i, SG_KMAP_ATOMIC);
> + if (IS_ERR(this_page)) {
> + scsicmd->result = DID_ERROR << 16;
> + return;
> + }
> +
>   memcpy(this_page, buf + bufind, sg[i].length);
> - kunmap_atomic(this_page_orig);
> + sg_unmap(sg + i, this_page, SG_KMAP_ATOMIC);
>   }
>   } else {
>   devdata = (struct visorhba_devdata *)scsidev->host-
> >hostdata;
> --
> 2.1.4

Re: [PATCH 09/22] dm-crypt: Make use of the new sg_map helper in 4 call sites

[Logan Gunthorpe]

On 14/04/17 02:39 AM, Christoph Hellwig wrote:
> On Thu, Apr 13, 2017 at 04:05:22PM -0600, Logan Gunthorpe wrote:
>> Very straightforward conversion to the new function in all four spots.
> 
> I think the right fix here is to switch dm-crypt to the ahash API
> that takes a scatterlist.

Hmm, well I'm not sure I understand the code enough to make that
conversion. But I was looking at it. One tricky bit seems to be that
crypt_iv_lmk_one adds a seed, skips the first 16 bytes in the page and
then hashes another 16 bytes from other data. What would you do
construct a new sgl for it and pass it to the ahash api?

The other thing is crypt_iv_lmk_post also seems to modify the page after
the hash with a  crypto_xor so you'd still need at least one kmap in there.

Logan

Re: export pcie_flr and remove copies of it in drivers

[Christoph Hellwig]

On Fri, Apr 14, 2017 at 09:51:48AM -0500, Bjorn Helgaas wrote:
> > Otherwise, I'd be glad to take the series given acks for the non-PCI
> > parts.  Just let me know.
> 
> I do already have a patch (c5e4f0192ad2 ("PCI: Avoid FLR for Intel
> 82579 NICs")) on my pci/virtualization branch that touches pcie_flr()
> and will conflict with this one.

I'll take a look and will resend on top of that branch.

Re: export pcie_flr and remove copies of it in drivers

[Christoph Hellwig]

On Fri, Apr 14, 2017 at 09:41:08AM -0500, Bjorn Helgaas wrote:
> Looks good to me (except the comment on probe).  If you want to apply
> the whole series via netdev or some non-PCI tree, here's my ack for
> the drivers/pci parts, assuming the probe thing is resolved:
> 
> Acked-by: Bjorn Helgaas 
> 
> Otherwise, I'd be glad to take the series given acks for the non-PCI
> parts.  Just let me know.

I guess the best would be if you take the first three patches (once
resent) for the PCI tree for 4.12, then we can do the other patches
in the next merge window.

Re: [PATCH 1/7] PCI: export pcie_flr

[Christoph Hellwig]

> s/pcie_has_flr/pcie_has_flr()/

Ok.

> This performs an FLR (if supported) always, regardless of "probe".
> I think it should look something like this instead:
> 
>   if (pcie_has_flr(dev)) {
> if (!probe)
>   pcie_flr(dev);
> rc = 0;
> goto done;
>   }

Indeed!

Re: [PATCH] padata: allow caller to control queue length

[Jason A. Donenfeld]

On Fri, Apr 14, 2017 at 9:57 AM, Steffen Klassert
 wrote:
> Why do we need this? As long as we don't have a user that needs a
> different limit, this patch adds just some useless code.

My [not-yet-mainlined] code wants it.

But more compellingly, padata simply isn't a very useful interface if
it contains random limits like that. Different consumers will want
different things. Pcrypt has been the only consumer for years and
years, despite it being in kernel/; it's probably a good idea to
figure out how to make this more attractive in general, since parallel
processing is a useful thing. The 1000 job limit is particularly
annoying for systems with tons and tons of cores; here you'd want the
ability for more jobs to be allowed at a time.

I'm also open to discussing other ways to handle limiting. But a hard
coded random number of 1000 seems wrong.

Re: [PATCH 03/22] libiscsi: Make use of new the sg_map helper function

[Logan Gunthorpe]

On 14/04/17 02:36 AM, Christoph Hellwig wrote:
> On Thu, Apr 13, 2017 at 04:05:16PM -0600, Logan Gunthorpe wrote:
>> Convert the kmap and kmap_atomic uses to the sg_map function. We now
>> store the flags for the kmap instead of a boolean to indicate
>> atomicitiy. We also propogate a possible kmap error down and create
>> a new ISCSI_TCP_INTERNAL_ERR error type for this.
> 
> Can you split out the new error handling into a separate prep patch
> which should go to the iscsi maintainers ASAP?
> 

Yes, I can do that. I'd just have thought they'd want to see the use
case for the new error before accepting a patch like that...

Logan

Re: [PATCH 01/22] scatterlist: Introduce sg_map helper functions

[Logan Gunthorpe]

On 14/04/17 02:35 AM, Christoph Hellwig wrote:
>> +
>>  static inline int is_dma_buf_file(struct file *);
>>  
>>  struct dma_buf_list {
> 
> I think the right fix here is to rename the operation to unmap_atomic
> and send out a little patch for that ASAP.

Ok, I can do that next week.

> I'd rather have separate functions for kmap vs kmap_atomic instead of
> the flags parameter.  And while you're at it just always pass the 0
> offset parameter instead of adding a wrapper..
> 
> Otherwise this looks good to me.

I settled on the flags because I thought the interface could be expanded
to do more things like automatically copy iomem to a bounce buffer (with
a flag). It'd also be possible to add things like vmap and
physical_address to the interface which would cover even more sg_page
users. All the implementations would then share the common offset
calculations, and switching between them becomes a matter of changing a
couple flags.

If you're still not convinced by the above arguments  then I'll change
it but I did have reasons for choosing to do it this way.

I am fine with removing the offset versions. I will make that change.

Thanks,

Logan

Re: [PATCH 04/22] target: Make use of the new sg_map function at 16 call sites (fwd)

[Julia Lawall]

It looks like >cmdr_lock should be released at line 512 if it has
not been released otherwise.  The lock was taken at line 438.

julia

-- Forwarded message --
Date: Fri, 14 Apr 2017 22:21:44 +0800
From: kbuild test robot <fengguang...@intel.com>
To: kbu...@01.org
Cc: Julia Lawall <julia.law...@lip6.fr>
Subject: Re: [PATCH 04/22] target: Make use of the new sg_map function at 16
call sites

Hi Logan,

[auto build test WARNING on scsi/for-next]
[also build test WARNING on v4.11-rc6]
[cannot apply to next-20170413]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Logan-Gunthorpe/Introduce-common-scatterlist-map-function/20170414-142518
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
:: branch date: 8 hours ago
:: commit date: 8 hours ago

>> drivers/target/target_core_user.c:512:2-8: preceding lock on line 438
   drivers/target/target_core_user.c:512:2-8: preceding lock on line 471

git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 78082134e7afdc59d744eb8d2def5c588e89c378
vim +512 drivers/target/target_core_user.c

7c9e7a6f Andy Grover  2014-10-01  432   
sizeof(struct tcmu_cmd_entry));
7c9e7a6f Andy Grover  2014-10-01  433   command_size = base_command_size
7c9e7a6f Andy Grover  2014-10-01  434   + 
round_up(scsi_command_size(se_cmd->t_task_cdb), TCMU_OP_ALIGN_SIZE);
7c9e7a6f Andy Grover  2014-10-01  435
7c9e7a6f Andy Grover  2014-10-01  436   WARN_ON(command_size & 
(TCMU_OP_ALIGN_SIZE-1));
7c9e7a6f Andy Grover  2014-10-01  437
7c9e7a6f Andy Grover  2014-10-01 @438   spin_lock_irq(>cmdr_lock);
7c9e7a6f Andy Grover  2014-10-01  439
7c9e7a6f Andy Grover  2014-10-01  440   mb = udev->mb_addr;
7c9e7a6f Andy Grover  2014-10-01  441   cmd_head = mb->cmd_head % 
udev->cmdr_size; /* UAM */
26418649 Sheng Yang   2016-02-26  442   data_length = 
se_cmd->data_length;
26418649 Sheng Yang   2016-02-26  443   if (se_cmd->se_cmd_flags & 
SCF_BIDI) {
26418649 Sheng Yang   2016-02-26  444   
BUG_ON(!(se_cmd->t_bidi_data_sg && se_cmd->t_bidi_data_nents));
26418649 Sheng Yang   2016-02-26  445   data_length += 
se_cmd->t_bidi_data_sg->length;
26418649 Sheng Yang   2016-02-26  446   }
554617b2 Andy Grover  2016-08-25  447   if ((command_size > 
(udev->cmdr_size / 2)) ||
554617b2 Andy Grover  2016-08-25  448   data_length > 
udev->data_size) {
554617b2 Andy Grover  2016-08-25  449   pr_warn("TCMU: Request 
of size %zu/%zu is too big for %u/%zu "
3d9b9555 Andy Grover  2016-08-25  450   "cmd ring/data 
area\n", command_size, data_length,
7c9e7a6f Andy Grover  2014-10-01  451   
udev->cmdr_size, udev->data_size);
554617b2 Andy Grover  2016-08-25  452   
spin_unlock_irq(>cmdr_lock);
554617b2 Andy Grover  2016-08-25  453   return 
TCM_INVALID_CDB_FIELD;
554617b2 Andy Grover  2016-08-25  454   }
7c9e7a6f Andy Grover  2014-10-01  455
26418649 Sheng Yang   2016-02-26  456   while 
(!is_ring_space_avail(udev, command_size, data_length)) {
7c9e7a6f Andy Grover  2014-10-01  457   int ret;
7c9e7a6f Andy Grover  2014-10-01  458   DEFINE_WAIT(__wait);
7c9e7a6f Andy Grover  2014-10-01  459
7c9e7a6f Andy Grover  2014-10-01  460   
prepare_to_wait(>wait_cmdr, &__wait, TASK_INTERRUPTIBLE);
7c9e7a6f Andy Grover  2014-10-01  461
7c9e7a6f Andy Grover  2014-10-01  462   pr_debug("sleeping for 
ring space\n");
7c9e7a6f Andy Grover  2014-10-01  463   
spin_unlock_irq(>cmdr_lock);
7c9e7a6f Andy Grover  2014-10-01  464   ret = 
schedule_timeout(msecs_to_jiffies(TCMU_TIME_OUT));
7c9e7a6f Andy Grover  2014-10-01  465   
finish_wait(>wait_cmdr, &__wait);
7c9e7a6f Andy Grover  2014-10-01  466   if (!ret) {
7c9e7a6f Andy Grover  2014-10-01  467   pr_warn("tcmu: 
command timed out\n");
02eb924f Andy Grover  2016-10-06  468   return 
TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
7c9e7a6f Andy Grover  2014-10-01  469   }
7c9e7a6f Andy Grover  2014-10-01  470
7c9e7a6f Andy Grover  2014-10-01  471   
spin_lock_irq(>cmdr_lock);
7c9e7a6f Andy Grover  2014-10-01  472
7c9e7a6f Andy Grover  2014-10-01  473   /* We dropped 
cmdr_lock, cmd_head is stale */
7c9e7a6f Andy Grover  2014-10-01  474   cmd_head = mb->cmd_head 
% udev->cmdr_size; /* UAM */
7c9e7a6f Andy Grover

Re: export pcie_flr and remove copies of it in drivers

[Bjorn Helgaas]

On Fri, Apr 14, 2017 at 9:41 AM, Bjorn Helgaas  wrote:
> On Thu, Apr 13, 2017 at 04:53:32PM +0200, Christoph Hellwig wrote:
>> Hi all,
>>
>> this exports the PCI layer pcie_flr helper, and removes various opencoded
>> copies of it.
>
> Looks good to me (except the comment on probe).  If you want to apply
> the whole series via netdev or some non-PCI tree, here's my ack for
> the drivers/pci parts, assuming the probe thing is resolved:
>
> Acked-by: Bjorn Helgaas 
>
> Otherwise, I'd be glad to take the series given acks for the non-PCI
> parts.  Just let me know.

I do already have a patch (c5e4f0192ad2 ("PCI: Avoid FLR for Intel
82579 NICs")) on my pci/virtualization branch that touches pcie_flr()
and will conflict with this one.

Re: export pcie_flr and remove copies of it in drivers

[Bjorn Helgaas]

On Thu, Apr 13, 2017 at 04:53:32PM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this exports the PCI layer pcie_flr helper, and removes various opencoded
> copies of it.

Looks good to me (except the comment on probe).  If you want to apply
the whole series via netdev or some non-PCI tree, here's my ack for
the drivers/pci parts, assuming the probe thing is resolved:

Acked-by: Bjorn Helgaas 

Otherwise, I'd be glad to take the series given acks for the non-PCI
parts.  Just let me know.

Bjorn

Re: [PATCH 1/7] PCI: export pcie_flr

[Bjorn Helgaas]

[+cc Alex]

On Thu, Apr 13, 2017 at 04:53:33PM +0200, Christoph Hellwig wrote:
> Currently we opencode the FLR sequence in lots of place, export a core
> helper instead.  We split out the probing for FLR support as all the
> non-core callers already know their hardware.
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  drivers/pci/pci.c   | 34 +-
>  include/linux/pci.h |  1 +
>  2 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7904d02ffdb9..3256a63c5d08 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3773,24 +3773,38 @@ static void pci_flr_wait(struct pci_dev *dev)
>(i - 1) * 100);
>  }
>  
> -static int pcie_flr(struct pci_dev *dev, int probe)
> +/**
> + * pcie_has_flr - check if a device supports function level resets
> + * @dev: device to check
> + *
> + * Returns true if the device advertises support for PCIe function level
> + * resets.
> + */
> +static bool pcie_has_flr(struct pci_dev *dev)
>  {
>   u32 cap;
>  
>   pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
> - if (!(cap & PCI_EXP_DEVCAP_FLR))
> - return -ENOTTY;
> -
> - if (probe)
> - return 0;
> + return cap & PCI_EXP_DEVCAP_FLR;
> +}
>  
> +/**
> + * pcie_flr - initiate a PCIe function level reset
> + * @dev: device to reset
> + *
> + * Initiate a function level reset on @dev.  The caller should ensure the
> + * device supports FLR before calling this function, e.g. by using the
> + * pcie_has_flr helper.

s/pcie_has_flr/pcie_has_flr()/

> + */
> +void pcie_flr(struct pci_dev *dev)
> +{
>   if (!pci_wait_for_pending_transaction(dev))
>   dev_err(>dev, "timed out waiting for pending transaction; 
> performing function level reset anyway\n");
>  
>   pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
>   pci_flr_wait(dev);
> - return 0;
>  }
> +EXPORT_SYMBOL_GPL(pcie_flr);
>  
>  static int pci_af_flr(struct pci_dev *dev, int probe)
>  {
> @@ -3971,9 +3985,11 @@ static int __pci_dev_reset(struct pci_dev *dev, int 
> probe)
>   if (rc != -ENOTTY)
>   goto done;
>  
> - rc = pcie_flr(dev, probe);
> - if (rc != -ENOTTY)
> + if (pcie_has_flr(dev)) {
> + pcie_flr(dev);
> + rc = 0;
>   goto done;
> + }

This performs an FLR (if supported) always, regardless of "probe".
I think it should look something like this instead:

  if (pcie_has_flr(dev)) {
if (!probe)
  pcie_flr(dev);
rc = 0;
goto done;
  }

>   rc = pci_af_flr(dev, probe);
>   if (rc != -ENOTTY)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index eb3da1a04e6c..f35e51eddad0 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1052,6 +1052,7 @@ int pcie_get_mps(struct pci_dev *dev);
>  int pcie_set_mps(struct pci_dev *dev, int mps);
>  int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
> enum pcie_link_width *width);
> +void pcie_flr(struct pci_dev *dev);
>  int __pci_reset_function(struct pci_dev *dev);
>  int __pci_reset_function_locked(struct pci_dev *dev);
>  int pci_reset_function(struct pci_dev *dev);
> -- 
> 2.11.0
> 

Re: [PATCH 09/22] dm-crypt: Make use of the new sg_map helper in 4 call sites

[Christoph Hellwig]

On Thu, Apr 13, 2017 at 04:05:22PM -0600, Logan Gunthorpe wrote:
> Very straightforward conversion to the new function in all four spots.

I think the right fix here is to switch dm-crypt to the ahash API
that takes a scatterlist.

Re: [PATCH 03/22] libiscsi: Make use of new the sg_map helper function

[Christoph Hellwig]

On Thu, Apr 13, 2017 at 04:05:16PM -0600, Logan Gunthorpe wrote:
> Convert the kmap and kmap_atomic uses to the sg_map function. We now
> store the flags for the kmap instead of a boolean to indicate
> atomicitiy. We also propogate a possible kmap error down and create
> a new ISCSI_TCP_INTERNAL_ERR error type for this.

Can you split out the new error handling into a separate prep patch
which should go to the iscsi maintainers ASAP?

Re: [PATCH 01/22] scatterlist: Introduce sg_map helper functions

[Christoph Hellwig]

> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 0007b79..b95934b 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -37,6 +37,9 @@
>  
>  #include 
>  
> +/* Prevent the highmem.h macro from aliasing ops->kunmap_atomic */
> +#undef kunmap_atomic
> +
>  static inline int is_dma_buf_file(struct file *);
>  
>  struct dma_buf_list {

I think the right fix here is to rename the operation to unmap_atomic
and send out a little patch for that ASAP.

> + *   Flags can be any of:
> + *   * SG_KMAP- Use kmap to create the mapping
> + *   * SG_KMAP_ATOMIC - Use kmap_atomic to map the page atommically.
> + *  Thus, the rules of that function apply: the cpu
> + *  may not sleep until it is unmaped.
> + *
> + *   Also, consider carefully whether this function is appropriate. It is
> + *   largely not recommended for new code and if the sgl came from another
> + *   subsystem and you don't know what kind of memory might be in the list
> + *   then you definitely should not call it. Non-mappable memory may be in
> + *   the sgl and thus this function may fail unexpectedly.
> + **/
> +static inline void *sg_map_offset(struct scatterlist *sg, size_t offset,
> +int flags)

I'd rather have separate functions for kmap vs kmap_atomic instead of
the flags parameter.  And while you're at it just always pass the 0
offset parameter instead of adding a wrapper..

Otherwise this looks good to me.

Re: [RFC PATCH v1 0/1] *** crypto: AF_ALG: add compression support ***

[Herbert Xu]

On Fri, Apr 14, 2017 at 12:04:53AM +0530, Abed Kamaluddin wrote:
> Hi Herbert,
> 
> This patch adds compression support to the AF_ALG interface exported by the 
> kernel crypto API. By extending AF_ALG, all compression algorithms of types 
> scomp and acomp, which the kernel crypto API allows access to, are now also
> accessible from userspace.
> 
> The new compression interface has been tested with both kernel software
> deflate(scomp) and HW accelerated ThunderX deflate(scomp) using the zpipe
> example application provided by zlib.

I think we should convert ipcomp over to the new interface first
in order to make sure that we don't need to change the interface
which would be hard to do once we export it to user-space.

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] padata: allow caller to control queue length

[Steffen Klassert]

On Thu, Apr 13, 2017 at 11:52:13AM +0200, Jason A. Donenfeld wrote:
> Allow users of padata to determine the queue length themselves, via this
> added helper function, so that we can later remove the hard-coded 1000-
> job limit. We thus add a helper function, and then move the limiting
> functionality to pcrypt-proper, since it was the only current consumer
> relying on the 1000-job limit. We do, however, impose a limit on padata
> so that the reference count does not have an integer overflow.
> 
> Signed-off-by: Jason A. Donenfeld 

Why do we need this? As long as we don't have a user that needs a
different limit, this patch adds just some useless code.

Re: [PATCH] padata: get_next is never NULL

[Steffen Klassert]

On Wed, Apr 12, 2017 at 10:40:19AM +0200, Jason A. Donenfeld wrote:
> Per Dan's static checker warning, the code that returns NULL was removed
> in 2010, so this patch updates the comments and fixes the code
> assumptions.
> 
> Signed-off-by: Jason A. Donenfeld 
> Reported-by: Dan Carpenter 

This looks ok,

Acked-by: Steffen Klassert