From owner-svn-src-head@FreeBSD.ORG Tue Jul 9 16:12:28 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id E4AAB80E; Tue, 9 Jul 2013 16:12:28 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) by mx1.freebsd.org (Postfix) with ESMTP id B94971AA7; Tue, 9 Jul 2013 16:12:28 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 7C2ABB948; Tue, 9 Jul 2013 12:12:27 -0400 (EDT) From: John Baldwin To: Garrett Cooper Subject: Re: svn commit: r253048 - in head/sys/ofed: drivers/net/mlx4 include/linux Date: Tue, 9 Jul 2013 11:20:21 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p25; KDE/4.5.5; amd64; ; ) References: <201307082125.r68LPDlY023493@svn.freebsd.org> <3FF894D3-ACDF-4796-A682-F9F9DD8C943D@gmail.com> In-Reply-To: <3FF894D3-ACDF-4796-A682-F9F9DD8C943D@gmail.com> MIME-Version: 1.0 Message-Id: <201307091120.22114.jhb@freebsd.org> Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Tue, 09 Jul 2013 12:12:27 -0400 (EDT) Cc: svn-src-head , svn-src-all , src-committers , shahark@mellanox.com X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Jul 2013 16:12:29 -0000 On Monday, July 08, 2013 6:13:22 pm Garrett Cooper wrote: > On Jul 8, 2013, at 2:25 PM, John Baldwin wrote: > > > Author: jhb > > Date: Mon Jul 8 21:25:12 2013 > > New Revision: 253048 > > URL: http://svnweb.freebsd.org/changeset/base/253048 > > > > Log: > > Allow mlx4 devices to switch from Ethernet to Infiniband (and vice versa): > > - Fix sysctl wrapper for sysfs attributes to properly handle new string > > values similar to sysctl_handle_string() (only copyin the user's > > supplied length and nul-terminate the string). > > - Don't check for a trailing newline when evaluating the desired operating > > mode of a mlx4 device. > > > > PR: kern/179999 > > Submitted by: Shahar Klein > > MFC after: 1 week > > Was there an issue with the patch I submitted via http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/174213 (basically solving the same problem, but only with the sysfs <-> sysctl(9) handler)? I was of the impression that sysfs on Linux always added on trailing newlines (but I could be wrong because I haven't used Linux at a dev level for ages). Thanks! I hadn't seen it. I had wondered if the '\n' issue was a generic sysfs thing. It sounds like it is and I'd be happy to revert the mlx4 change and alter the sysfs bits to manage the newline directly if that is more appropriate. I'd also like this to use sysctl_handle_string() if at all possible. Are you in a position to test patches still? If so, maybe give this a whirl. It's similar to yours except it uses sysctl_handle_string() and strlcat() rather than continuing to do things by hand. It also outputs an empty string to userland if the attribute doesn't have a show method (your version would never pass out an old string in that case unlike the original code). Index: drivers/net/mlx4/main.c =================================================================== --- drivers/net/mlx4/main.c (revision 253096) +++ drivers/net/mlx4/main.c (working copy) @@ -479,11 +479,11 @@ int i; int err = 0; - if (!strcmp(buf, "ib")) + if (!strcmp(buf, "ib\n")) info->tmp_type = MLX4_PORT_TYPE_IB; - else if (!strcmp(buf, "eth")) + else if (!strcmp(buf, "eth\n")) info->tmp_type = MLX4_PORT_TYPE_ETH; - else if (!strcmp(buf, "auto")) + else if (!strcmp(buf, "auto\n")) info->tmp_type = MLX4_PORT_TYPE_AUTO; else { mlx4_err(mdev, "%s is not supported port type\n", buf); Index: include/linux/sysfs.h =================================================================== --- include/linux/sysfs.h (revision 253096) +++ include/linux/sysfs.h (working copy) @@ -81,37 +81,35 @@ kobj = arg1; attr = (struct attribute *)arg2; - buf = (void *)get_zeroed_page(GFP_KERNEL); - len = 1; /* Copy out a NULL byte at least. */ if (kobj->ktype == NULL || kobj->ktype->sysfs_ops == NULL) return (ENODEV); - ops = kobj->ktype->sysfs_ops; + buf = (void *)get_zeroed_page(GFP_KERNEL); if (buf == NULL) return (ENOMEM); + ops = kobj->ktype->sysfs_ops; if (ops->show) { len = ops->show(kobj, attr, buf); /* - * It's valid not to have a 'show' so we just return 1 byte - * of NULL. + * It's valid to not have a 'show' so just return an + * empty string. */ if (len < 0) { error = -len; - len = 1; if (error != EIO) goto out; } + + /* Trim trailing newline. */ + len--; + buf[len] = '\0'; } - error = SYSCTL_OUT(req, buf, len); - if (error || !req->newptr || ops->store == NULL) + + /* Leave one trailing byte to append a newline. */ + error = sysctl_handle_string(oidp, buf, PAGE_SIZE - 1, req); + if (error != 0 || req->newptr == NULL || ops->store == NULL) goto out; - len = req->newlen - req->newidx; - if (len >= PAGE_SIZE) - error = EINVAL; - else - error = SYSCTL_IN(req, buf, len); - if (error) - goto out; - ((char *)buf)[len] = '\0'; + len = strlcat(buf, "\n", PAGE_SIZE); + KASSERT(len < PAGE_SIZE, ("new attribute truncated")); len = ops->store(kobj, attr, buf, len); if (len < 0) error = -len; -- John Baldwin