Message-ID: <20161219122923.GA18379@hardcore>
Date:   Mon, 19 Dec 2016 13:29:23 +0100
From:   Jan Glauber <jan.glauber@...iumnetworks.com>
To:     Corentin Labbe <clabbe.montjoie@...il.com>
CC:     Herbert Xu <herbert@...dor.apana.org.au>,
        <linux-crypto@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Mahipal Challa <Mahipal.Challa@...ium.com>,
        Vishnu Nair <Vishnu.Nair@...ium.com>
Subject: Re: [RFC PATCH 1/3] crypto: zip - Add ThunderX ZIP driver core

Hi Corentin,

thanks for your review! Your comments all look reasonable to me,
Mahipal will address them.

Since I posted this series at the beginning of the merge window
I'd like to wait for some more time before we post an updated version.

--Jan

On Tue, Dec 13, 2016 at 02:39:00PM +0100, Corentin Labbe wrote:
> Hello
> 
> I have some comment below
> 
> On Mon, Dec 12, 2016 at 04:04:37PM +0100, Jan Glauber wrote:
> > From: Mahipal Challa <Mahipal.Challa@...ium.com>
> > 
> [...]
> > --- a/drivers/crypto/Makefile
> > +++ b/drivers/crypto/Makefile
> > @@ -27,6 +27,7 @@ obj-$(CONFIG_CRYPTO_DEV_MXC_SCC) += mxc-scc.o
> >  obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o
> >  obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/
> >  obj-$(CONFIG_CRYPTO_DEV_QAT) += qat/
> > +obj-$(CONFIG_CRYPTO_DEV_CAVIUM_ZIP) += cavium/
> >  obj-$(CONFIG_CRYPTO_DEV_QCE) += qce/
> >  obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
> >  obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/
> 
> Try to keep some alphabetical order
> 
> [...]
> > +/* ZIP invocation result completion status codes */
> > +#define ZIP_NOTDONE		0x0
> > +
> > +/* Successful completion. */
> > +#define ZIP_SUCCESS		0x1
> > +
> > +/* Output truncated */
> > +#define ZIP_DTRUNC		0x2
> > +
> > +/* Dynamic Stop */
> > +#define ZIP_DYNAMIC_STOP	0x3
> > +
> > +/* Uncompress ran out of input data when IWORD0[EF] was set */
> > +#define ZIP_ITRUNC		0x4
> > +
> > +/* Uncompress found the reserved block type 3 */
> > +#define ZIP_RBLOCK		0x5
> > +
> > +/* Uncompress found LEN != ZIP_NLEN in an uncompressed block in the input */
> > +#define ZIP_NLEN		0x6
> > +
> > +/* Uncompress found a bad code in the main Huffman codes. */
> > +#define ZIP_BADCODE		0x7
> > +
> > +/* Uncompress found a bad code in the 19 Huffman codes encoding lengths. */
> > +#define ZIP_BADCODE2	        0x8
> > +
> > +/* Compress found a zero-length input. */
> > +#define ZIP_ZERO_LEN	        0x9
> > +
> > +/* The compress or decompress encountered an internal parity error. */
> > +#define ZIP_PARITY		0xA
> 
> Perhaps all errors could begin with ZIP_ERR_xxx ?
> 
> [...]
> > +static inline u64 zip_depth(void)
> > +{
> > +	struct zip_device *zip_dev = zip_get_device(zip_get_node_id());
> > +
> > +	if (!zip_dev)
> > +		return -ENODEV;
> > +
> > +	return zip_dev->depth;
> > +}
> 
> This function is not used.
> 
> > +
> > +static inline u64 zip_onfsize(void)
> > +{
> > +	struct zip_device *zip_dev = zip_get_device(zip_get_node_id());
> > +
> > +	if (!zip_dev)
> > +		return -ENODEV;
> > +
> > +	return zip_dev->onfsize;
> > +}
> 
> Same for this
> 
> > +
> > +static inline u64 zip_ctxsize(void)
> > +{
> > +	struct zip_device *zip_dev = zip_get_device(zip_get_node_id());
> > +
> > +	if (!zip_dev)
> > +		return -ENODEV;
> > +
> > +	return zip_dev->ctxsize;
> > +}
> 
> Again
> 
> [...]
> > +
> > +/*
> > + * Allocates new ZIP device structure
> > + * Returns zip_device pointer or NULL if cannot allocate memory for zip_device
> > + */
> > +static struct zip_device *zip_alloc_device(struct pci_dev *pdev)
> 
> pdev is not used, so you can remove it from arglist.
> Or keep it and use devm_kzalloc for allocating zip
> 
> > +{
> > +	struct zip_device *zip = NULL;
> > +	int idx = 0;
> > +
> > +	for (idx = 0; idx < MAX_ZIP_DEVICES; idx++) {
> > +		if (!zip_dev[idx])
> > +			break;
> > +	}
> > +
> > +	zip = kzalloc(sizeof(*zip), GFP_KERNEL);
> > +
> > +	if (!zip)
> > +		return NULL;
> > +
> > +	zip_dev[idx] = zip;
> > +	zip->index = idx;
> > +	return zip;
> > +}
> 
> [...]
> > +static int zip_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct zip_device *zip = NULL;
> > +	int    err;
> > +
> > +	zip_dbg_enter();
> > +
> > +	zip = zip_alloc_device(pdev);
> > +
> > +	if (!zip)
> > +		return -ENOMEM;
> > +
> > +	pr_info("Found ZIP device %d %x:%x on Node %d\n", zip->index,
> > +		pdev->vendor, pdev->device, dev_to_node(dev));
> 
> You use lots of pr_info, why not using more dev_info/dev_err ?
> 
> > +
> > +	zip->pdev = pdev;
> > +
> > +	pci_set_drvdata(pdev, zip);
> > +
> > +	err = pci_enable_device(pdev);
> > +	if (err) {
> > +		zip_err("Failed to enable PCI device");
> > +		goto err_free_device;
> > +	}
> > +
> > +	err = pci_request_regions(pdev, DRV_NAME);
> > +	if (err) {
> > +		zip_err("PCI request regions failed 0x%x", err);
> > +		goto err_disable_device;
> > +	}
> > +
> > +	err = pci_set_dma_mask(pdev, DMA_BIT_MASK(48));
> > +	if (err) {
> > +		dev_err(dev, "Unable to get usable DMA configuration\n");
> > +		goto err_release_regions;
> > +	}
> > +
> > +	err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(48));
> > +	if (err) {
> > +		dev_err(dev, "Unable to get 48-bit DMA for allocations\n");
> > +		goto err_release_regions;
> > +	}
> > +
> > +	/* MAP configuration registers */
> > +	zip->reg_base = pci_ioremap_bar(pdev, PCI_CFG_ZIP_PF_BAR0);
> > +	if (!zip->reg_base) {
> > +		zip_err("ZIP: Cannot map BAR0 CSR memory space, aborting");
> > +		err = -ENOMEM;
> > +		goto err_release_regions;
> > +	}
> > +
> > +	/* Initialize ZIP Hardware */
> > +	err = zip_init_hw(zip);
> > +	if (err)
> > +		goto err_release_regions;
> > +
> > +	return 0;
> > +
> > +err_release_regions:
> > +	if (zip->reg_base)
> > +		iounmap(zip->reg_base);
> > +	pci_release_regions(pdev);
> > +
> > +err_disable_device:
> > +	pci_disable_device(pdev);
> > +
> > +err_free_device:
> > +	pci_set_drvdata(pdev, NULL);
> > +
> > +	/* remove zip_dev from zip_device list, free the zip_device memory */
> > +	zip_dev[zip->index] = NULL;
> > +	kfree(zip);
> > +
> > +	zip_dbg_exit();
> > +	return err;
> > +}
> 
> [...]
> > +static int __init zip_init_module(void)
> > +{
> > +	int ret;
> > +
> > +	memset(&zip_dev, 0, sizeof(zip_dev));
> 
> A static variable is already zeroed
> 
> > +
> > +	zip_msg("%s\n", DRV_NAME);
> > +
> > +	ret = pci_register_driver(&zip_driver);
> > +	if (ret < 0) {
> > +		zip_err("ZIP: pci_register_driver() returned %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* Register with the Kernel Crypto Interface */
> > +	ret = zip_register_compression_device();
> > +	if (ret < 0) {
> > +		zip_err("ZIP: Kernel Crypto Registration failed\n");
> > +		return 1;
> 
> 1 is not a good error code. And you quit without unregistering.
> 
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void __exit zip_cleanup_module(void)
> > +{
> > +	/* Unregister this driver for pci zip devices */
> > +	pci_unregister_driver(&zip_driver);
> > +
> > +	/* Unregister from the kernel crypto interface */
> > +	zip_unregister_compression_device();
> 
> I think you must do the opposite. (unregister crypto first)
> 
> > +
> > +	pr_info("ThunderX-ZIP driver is removed successfully\n");
> > +}
> > +
> > +module_init(zip_init_module);
> > +module_exit(zip_cleanup_module);
> 
> Why not using module_pci_driver ?
> 
> [...]
> > +/**
> > + * enum zip_comp_e - ZIP Completion Enumeration, enumerates the values of
> > + * ZIP_ZRES_S[COMPCODE].
> > + */
> > +enum zip_comp_e {
> > +	ZIP_COMP_E_BADCODE = 0x7,
> > +	ZIP_COMP_E_BADCODE2 = 0x8,
> > +	ZIP_COMP_E_DTRUNC = 0x2,
> > +	ZIP_COMP_E_FATAL = 0xb,
> > +	ZIP_COMP_E_ITRUNC = 0x4,
> > +	ZIP_COMP_E_NLEN = 0x6,
> > +	ZIP_COMP_E_NOTDONE = 0x0,
> > +	ZIP_COMP_E_PARITY = 0xa,
> > +	ZIP_COMP_E_RBLOCK = 0x5,
> > +	ZIP_COMP_E_STOP = 0x3,
> > +	ZIP_COMP_E_SUCCESS = 0x1,
> > +	ZIP_COMP_E_ZERO_LEN = 0x9,
> > +	ZIP_COMP_E_ENUM_LAST = 0xc,
> 
> Why not using already declared define ? ZIP_COMP_E_BADCODE = ZIP_BADCODE 
> 
> Regards
> Corentin Labbe

Powered by blists - more mailing lists