Received: by 2002:a05:6a10:f347:0:0:0:0 with SMTP id d7csp46024pxu; Tue, 24 Nov 2020 18:08:24 -0800 (PST) X-Google-Smtp-Source: ABdhPJyW5Zarn80RcOo25+ZA3NzK0Ym2N6BHIBTPYZl0/bcBlv3oYlVmLFN+weOctMsyFLYZgQjg X-Received: by 2002:a50:d78f:: with SMTP id w15mr1404633edi.227.1606270104694; Tue, 24 Nov 2020 18:08:24 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1606270104; cv=none; d=google.com; s=arc-20160816; b=HETur5GEVdzQD2zx+JKpfIs6ZOkd5gryrG8iPa+WhM7wnctFqcERxjYpsLn+YL9gqf h9sGqSUCO5GcIQcZwAPTTFhGEwMcA6Q20yekDHWsSTNrg2dVLh1Z8+/Xa+4XBs+D8w50 x7pgn26ImshOlEYXc5A/2kMizoinMhOSmv0TdERHAHKW+JmoIlEB7Ddw4ZA3ElWXX4mx Bn9UOqDKlqCvWziaqNlauvmebYe6hsYg0jTE1CPbf2V4hRCbS378+qEy8pf5V4/f8DxK StFpY1bnrZi4IImrSNPuzD74kZLxzKbWEgrxCwJ8iUCoTZIZ4hICH9PbJrYCu+30UoYA xuqw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date; bh=UV5dAkTriHZWlO3RZNeKzyPFwM32bAdjh6TvkdygNLU=; b=soz3RpQLIwyd9shjFpGNVbdeMGEqElWQQqXenPe7pl614E9jpIhA03iQNikNerRxJz NKEiCX2aMQhiZ3KkTdwCjrk6IbKHv5cJz0i+Qjy7bJQ3vBnnU+UgOC38Q7TPKIxCK77S 0/xPGuZYu1rM8sjrNtu4mqBTo5R4uTs4tXIwnKzheZRa7rk/GM2+Y/A/saIKe3+tkDGK rUZZUrPImbQphErKYj9uE+yDaD/UxL0KOwM0Vhkf5/1P8rnBdkumKKh/6XXrpBkGkE/T TTczAANWYzGS8vONQ9MV37cfgYv3rNL2maVur6eK3AjfCM+xwme27z05ALxmTiGgGRIg ZiHg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id i12si429543ejb.749.2020.11.24.18.08.01; Tue, 24 Nov 2020 18:08:24 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2388738AbgKXOP4 (ORCPT + 99 others); Tue, 24 Nov 2020 09:15:56 -0500 Received: from mail-ed1-f65.google.com ([209.85.208.65]:45684 "EHLO mail-ed1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2388732AbgKXOP4 (ORCPT ); Tue, 24 Nov 2020 09:15:56 -0500 Received: by mail-ed1-f65.google.com with SMTP id q3so20993129edr.12; Tue, 24 Nov 2020 06:15:51 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=UV5dAkTriHZWlO3RZNeKzyPFwM32bAdjh6TvkdygNLU=; b=iZLbxa6RVLQ/eGQQUr6tQnhewRxsLVz4WSBlxhG9soMsum1SJoVaZdhJGEtcjxheWy wKRP+fOtvEqU+EJfCsWXjC7RYvy3kS5LnqjJzUQSNSLmK4WzvR8LAdxFqurb3w2bxk33 yRhuG/jVKWo+sPpA+TwVR5c1YvHcBxu4zSCbriBREGg48/kZop2tJorj3d85y/G00+A5 E2WYxUdfYpI6Tf8sHFrdbG2CYqfqdrirVbMvAtjN3FFF71ayC1Kh56qrV6MXdXXP8dpO yeB/Wuz5Ry+R2U47SmSo/nE6XBtHPmWyCgNF3Pizpqhi6POMLDCJT44s75O5C8SDQLy3 ur+A== X-Gm-Message-State: AOAM532ezJ3KEOBsXgGE/EO3LuQaogvj/9ape5QjJvktz2yTMkbjIbML 1ypE4xEBAgyRSoZ47nJaKpCJyRcJqlE= X-Received: by 2002:a05:6402:32c:: with SMTP id q12mr4192496edw.85.1606227350694; Tue, 24 Nov 2020 06:15:50 -0800 (PST) Received: from kozik-lap (adsl-84-226-167-205.adslplus.ch. [84.226.167.205]) by smtp.googlemail.com with ESMTPSA id m3sm7039993edj.22.2020.11.24.06.15.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 24 Nov 2020 06:15:49 -0800 (PST) Date: Tue, 24 Nov 2020 15:15:47 +0100 From: "krzk@kernel.org" To: Bongsu Jeon Cc: Bongsu Jeon , "kuba@kernel.org" , "linux-kernel@vger.kernel.org" , "linux-nfc@lists.01.org" , "netdev@vger.kernel.org" Subject: Re: [PATCH net-next 2/2] net: nfc: s3fwrn5: Support a UART interface Message-ID: <20201124141547.GA3316@kozik-lap> References: <20201123075658epcms2p5a6237314f7a72a2556545d3f96261c93@epcms2p5> <20201123081940.GA9323@kozik-lap> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Nov 24, 2020 at 09:05:52PM +0900, Bongsu Jeon wrote: > On Mon, Nov 23, 2020 at 5:55 PM krzk@kernel.org wrote: > > > +static enum s3fwrn5_mode s3fwrn82_uart_get_mode(void *phy_id) > > > +{ > > > + struct s3fwrn82_uart_phy *phy = phy_id; > > > + enum s3fwrn5_mode mode; > > > + > > > + mutex_lock(&phy->mutex); > > > + mode = phy->mode; > > > + mutex_unlock(&phy->mutex); > > > + return mode; > > > +} > > > > All this duplicates I2C version. You need to start either reusing common > > blocks. > > > > Okay. I will do refactoring on i2c.c and uart.c to make common blocks. > is it okay to separate a patch for it? Yes, that would be the best - refactor the driver to split some common methods and then in next patch add new s3fwrn82 UART driver. > > > + > > > +static int s3fwrn82_uart_write(void *phy_id, struct sk_buff *out) > > > +{ > > > + struct s3fwrn82_uart_phy *phy = phy_id; > > > + int err; > > > + > > > + err = serdev_device_write(phy->ser_dev, > > > + out->data, out->len, > > > + MAX_SCHEDULE_TIMEOUT); > > > + if (err < 0) > > > + return err; > > > + > > > + return 0; > > > +} > > > + > > > +static const struct s3fwrn5_phy_ops uart_phy_ops = { > > > + .set_wake = s3fwrn82_uart_set_wake, > > > + .set_mode = s3fwrn82_uart_set_mode, > > > + .get_mode = s3fwrn82_uart_get_mode, > > > + .write = s3fwrn82_uart_write, > > > +}; > > > + > > > +static int s3fwrn82_uart_read(struct serdev_device *serdev, > > > + const unsigned char *data, > > > + size_t count) > > > +{ > > > + struct s3fwrn82_uart_phy *phy = serdev_device_get_drvdata(serdev); > > > + size_t i; > > > + > > > + for (i = 0; i < count; i++) { > > > + skb_put_u8(phy->recv_skb, *data++); > > > + > > > + if (phy->recv_skb->len < S3FWRN82_NCI_HEADER) > > > + continue; > > > + > > > + if ((phy->recv_skb->len - S3FWRN82_NCI_HEADER) > > > + < phy->recv_skb->data[S3FWRN82_NCI_IDX]) > > > + continue; > > > + > > > + s3fwrn5_recv_frame(phy->ndev, phy->recv_skb, phy->mode); > > > + phy->recv_skb = alloc_skb(NCI_SKB_BUFF_LEN, GFP_KERNEL); > > > + if (!phy->recv_skb) > > > + return 0; > > > + } > > > + > > > + return i; > > > +} > > > + > > > +static struct serdev_device_ops s3fwrn82_serdev_ops = { > > > > const > > > > > + .receive_buf = s3fwrn82_uart_read, > > > + .write_wakeup = serdev_device_write_wakeup, > > > +}; > > > + > > > +static const struct of_device_id s3fwrn82_uart_of_match[] = { > > > + { .compatible = "samsung,s3fwrn82-uart", }, > > > + {}, > > > +}; > > > +MODULE_DEVICE_TABLE(of, s3fwrn82_uart_of_match); > > > + > > > +static int s3fwrn82_uart_parse_dt(struct serdev_device *serdev) > > > +{ > > > + struct s3fwrn82_uart_phy *phy = serdev_device_get_drvdata(serdev); > > > + struct device_node *np = serdev->dev.of_node; > > > + > > > + if (!np) > > > + return -ENODEV; > > > + > > > + phy->gpio_en = of_get_named_gpio(np, "en-gpios", 0); > > > + if (!gpio_is_valid(phy->gpio_en)) > > > + return -ENODEV; > > > + > > > + phy->gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0); > > > > You should not cast it it unsigned int. I'll fix the s3fwrn5 from which > > you copied this apparently. > > > > Okay. I will fix it. > > > > + if (!gpio_is_valid(phy->gpio_fw_wake)) > > > + return -ENODEV; > > > + > > > + return 0; > > > +} > > > + > > > +static int s3fwrn82_uart_probe(struct serdev_device *serdev) > > > +{ > > > + struct s3fwrn82_uart_phy *phy; > > > + int ret = -ENOMEM; > > > + > > > + phy = devm_kzalloc(&serdev->dev, sizeof(*phy), GFP_KERNEL); > > > + if (!phy) > > > + goto err_exit; > > > + > > > + phy->recv_skb = alloc_skb(NCI_SKB_BUFF_LEN, GFP_KERNEL); > > > + if (!phy->recv_skb) > > > + goto err_free; > > > + > > > + mutex_init(&phy->mutex); > > > + phy->mode = S3FWRN5_MODE_COLD; > > > + > > > + phy->ser_dev = serdev; > > > + serdev_device_set_drvdata(serdev, phy); > > > + serdev_device_set_client_ops(serdev, &s3fwrn82_serdev_ops); > > > + ret = serdev_device_open(serdev); > > > + if (ret) { > > > + dev_err(&serdev->dev, "Unable to open device\n"); > > > + goto err_skb; > > > + } > > > + > > > + ret = serdev_device_set_baudrate(serdev, 115200); > > > > Why baudrate is fixed? > > > > RN82 NFC chip only supports 115200 baudrate for UART. OK, I guess it could be extended in the future for other frequencies, if needed. > > > > + if (ret != 115200) { > > > + ret = -EINVAL; > > > + goto err_serdev; > > > + } > > > + > > > + serdev_device_set_flow_control(serdev, false); > > > + > > > + ret = s3fwrn82_uart_parse_dt(serdev); > > > + if (ret < 0) > > > + goto err_serdev; > > > + > > > + ret = devm_gpio_request_one(&phy->ser_dev->dev, > > > + phy->gpio_en, > > > + GPIOF_OUT_INIT_HIGH, > > > + "s3fwrn82_en"); > > > > This is weirdly wrapped. > > > > Did you ask about devem_gpio_request_one function's parenthesis and parameters? > If it is right, I changed it after i ran the checkpatch.pl --strict and > i saw message like the alignment should match open parenthesis. Yeah, but it does not mean to wrap after each argument. It should be something like: ret = devm_gpio_request_one(&phy->ser_dev->dev, phy->gpio_en, GPIOF_OUT_INIT_HIGH, "s3fwrn82_en"); > > > > + if (ret < 0) > > > + goto err_serdev; > > > + > > > + ret = devm_gpio_request_one(&phy->ser_dev->dev, > > > + phy->gpio_fw_wake, > > > + GPIOF_OUT_INIT_LOW, > > > + "s3fwrn82_fw_wake"); > > > + if (ret < 0) > > > + goto err_serdev; > > > + > > > + ret = s3fwrn5_probe(&phy->ndev, phy, &phy->ser_dev->dev, &uart_phy_ops); > > > + if (ret < 0) > > > + goto err_serdev; > > > + > > > + return ret; > > > + > > > +err_serdev: > > > + serdev_device_close(serdev); > > > +err_skb: > > > + kfree_skb(phy->recv_skb); > > > +err_free: > > > + kfree(phy); > > > > Eee.... why? Did you test this code? > > > > I didn't test this code. i just added this code as defense code. > If the error happens, then allocated memory and device will be free > according to the fail case. Really, this won't work. It's kind of obvious why... You cannot use kfree() on memory which is not allocated with kzalloc(). Or IOW, you cannot use it if it is being freed by devm. I doubt that you tested either this or the remove callback because if you did test it, you would see easily: [ 145.018200] Unable to handle kernel paging request at virtual address ffffffed [ 145.025428] pgd = 67e71ef8 [ 145.027774] [ffffffed] *pgd=6fffd861, *pte=00000000, *ppte=00000000 [ 145.034052] Internal error: Oops: 837 [#1] PREEMPT SMP ARM [ 145.039278] Modules linked in: s5p_mfc exynos_gsc s5p_jpeg v4l2_mem2mem videobuf2_dma_contig videobuf2_memops videobuf2_v4l2 videobuf2_common videodev mc [ 145.052987] CPU: 2 PID: 325 Comm: bash Tainted: G W 5.10.0-rc3-next-20201113-00008-g62dd0da04641-dirty #79 [ 145.063883] Hardware name: Samsung Exynos (Flattened Device Tree) [ 145.069971] PC is at devres_remove+0x9c/0xb4 [ 145.074180] LR is at devres_remove+0x78/0xb4 [ 145.078418] pc : [] lr : [] psr: a0010093 [ 145.084666] sp : c6af7de0 ip : 00000001 fp : c2dd16e4 [ 145.089861] r10: c2dd16c0 r9 : 60010013 r8 : c05894a8 [ 145.095060] r7 : c058b128 r6 : c4bbc100 r5 : c2dd1410 r4 : c440d5c0 [ 145.101564] r3 : ffffffed r2 : c4bbc100 r1 : 60010013 r0 : c2dd16c0 [ 145.108068] Flags: NzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none [ 145.115260] Control: 10c5387d Table: 46aa006a DAC: 00000051 [ 145.120979] Process bash (pid: 325, stack limit = 0x97d30bf6) [ 145.126696] Stack: (0xc6af7de0 to 0xc6af8000) [ 145.131039] 7de0: c4bbc100 c2dd1410 c058b128 c2dd1410 c440d5c0 c39a3400 c1305f08 c1348d28 [ 145.139185] 7e00: 0000002b c06a6a54 c2dd1410 00000000 c440d5c0 c058ab94 c2dd1410 c06ccd04 [ 145.147333] 7e20: c2dd1410 c19ab618 00000000 c19ab620 c39a3400 c06a1ff8 00000000 c2dd1524 [ 145.155476] 7e40: c1c16800 c2dd1410 c1305f08 c1305f08 c1348d28 c39a3400 c6af7f78 c39a3410 [ 145.163621] 7e60: c1c16800 c06a2624 c1305f08 c1305f08 0000000c c2dd1410 00000000 c1305f08 [ 145.171769] 7e80: 0000000c c39a3400 c6af7f78 c39a3410 c1c16800 c06a29ec c2dd1410 c1305f08 [ 145.179914] 7ea0: c12f0410 c06a0728 0000000c c4bbc140 00000000 00000000 c39a3400 c0381968 [ 145.188058] 7ec0: 00000000 00000000 00000000 c5164640 0000000c 005e0da0 c6af7f78 c02d6120 [ 145.196202] 7ee0: c038185c c02d5b70 00000001 00000000 c02d6120 00000000 00000000 c133c27a [ 145.204353] 7f00: c39f5690 c127b15c c116e588 c0b40ef0 c133c14f c3af4450 c39f5670 00000000 [ 145.212497] 7f20: c3af4450 c018ea90 c120958c c39f5670 c3af4450 c116e588 c02f9988 c0197a00 [ 145.220639] 7f40: c116e588 c0b40ef0 00000003 c1208ec8 00000000 c5164640 c5164640 00000000 [ 145.228784] 7f60: 00000000 005e0da0 0000000c 00000004 00000000 c02d6120 00000000 00000000 [ 145.236931] 7f80: 00000006 c1208ec8 c5164640 0000000c 005e0da0 b6fec680 00000004 c0100244 [ 145.245077] 7fa0: c6af6000 c0100060 0000000c 005e0da0 00000001 005e0da0 0000000c 00000000 [ 145.253222] 7fc0: 0000000c 005e0da0 b6fec680 00000004 b6f6310c b6f62c74 00000000 00000000 [ 145.261370] 7fe0: 00000498 bef78290 b6e84ea4 b6ee2300 60010010 00000001 00000000 00000000 [ 145.269555] [] (devres_remove) from [] (devres_release+0x10/0x3c) [ 145.277340] [] (devres_release) from [] (devm_pinctrl_put+0x20/0x44) [ 145.285388] [] (devm_pinctrl_put) from [] (pinctrl_bind_pins+0xd0/0x27c) [ 145.293786] [] (pinctrl_bind_pins) from [] (really_probe+0x90/0x4f0) [ 145.301842] [] (really_probe) from [] (driver_probe_device+0x78/0x1e0) [ 145.310069] [] (driver_probe_device) from [] (device_driver_attach+0x58/0x60) [ 145.318936] [] (device_driver_attach) from [] (bind_store+0x8c/0x100) [ 145.327074] [] (bind_store) from [] (kernfs_fop_write+0x10c/0x230) [ 145.334960] [] (kernfs_fop_write) from [] (vfs_write+0xcc/0x524) [ 145.342658] [] (vfs_write) from [] (ksys_write+0x64/0xf0) [ 145.349759] [] (ksys_write) from [] (ret_fast_syscall+0x0/0x2c) [ 145.357377] Exception stack(0xc6af7fa8 to 0xc6af7ff0) [ 145.362395] 7fa0: 0000000c 005e0da0 00000001 005e0da0 0000000c 00000000 [ 145.370551] 7fc0: 0000000c 005e0da0 b6fec680 00000004 b6f6310c b6f62c74 00000000 00000000 [ 145.378699] 7fe0: 00000498 bef78290 b6e84ea4 b6ee2300 [ 145.383719] Code: e1a0000a e5942000 e1a01009 e5823004 (e5832000) [ 145.389795] ---[ end trace 769f050185612ec3 ]--- Please fix the double-free. Best regards, Krzysztof