linux-kernel - RE: [PATCH V3 1/2] NTB: Add AMD PCI-Express NTB driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 18 Jan 2016 06:47:36 +0000
From:	"Yu, Xiangliang" <Xiangliang.Yu@....com>
To:	Allen Hubbe <Allen.Hubbe@....com>,
	"jdmason@...zu.us" <jdmason@...zu.us>,
	"dave.jiang@...el.com" <dave.jiang@...el.com>,
	"linux-ntb@...glegroups.com" <linux-ntb@...glegroups.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:	SPG_Linux_Kernel <SPG_Linux_Kernel@....com>
Subject: RE: [PATCH V3 1/2] NTB: Add AMD PCI-Express NTB driver


> > In here, i list all features AMD NTB support, not patch. Please see my
> > word again.
> 
> This is the commit message for this patch.  It would be confusing to have
> features mentioned in the commit message not implemented, and no
> explanation why not.

Ok, I'll change it.

> > > What is SMU?
> >
> > System management unit, please reference AMD manual.
> 
> Thanks, but it is your responsibility to support this driver, and respond to
> questions like this.  I didn't write it, I can't test it, and I don't have a hardware
> manual.
> 
> If this is an ack for the system management unit, is it acknowledging a
> watchdog on the SMU?  Does this ack have to do with the flush or power
> management functions?  If it is a watchdog, what implications are there for
> unloading your driver - does the watchdog just time out, and then what,
> reset the device?  I'm speculating (guessing) again, but that's really the best I
> can do without your authoritative answers, or a hardware manual.
> 
> Where can the hardware manual be found?  Has it been published, or is it
> AMD confidential?
> 
> http://search.amd.com/en-us/Pages/results-all.aspx#k=zeppelin
> http://search.amd.com/en-us/Pages/results-all.aspx#k=ntb
> 
> "Nothing here matches your search"

SMU is a big system, which including hardware and firmware. I think it is
Transport to NTB, just use its interface it expose and following NTB hardware
Specification. Right now, the specification hasn't been public released. I think
You need to request NDA( Non Disclosure Agreeement) license to review it.

> > > I would prefer to see #define, but this works... There are good
> > arguments to
> > > be made for compiler constants vs the preprocessor.  My preference
> > just
> > > comes from what I've observed about how other drivers are written,
> > > and #define for this stuff seems to be the convention.
> >
> > I have lots of SATA/Block layer experience, this following AHCI coding
> > style.
> > Compiler will check enum variable, not for macro.
> > Actually, there are lots of similar definition in kernel, such as
> > block layer code.
> 
> Thanks.  I'm ok with this.
> 
> > > It's unusual, sometimes dangerous, to use the static keyword in a .h
> > file
> >
> > Actually, I put it in .c file in first version, I think it make code
> > clear if moving it to .h file. Yes, I can remove all these definitions
> > by rearranging .c file.
> >
> > And please speak out all your concern in this time, you know, I had
> > spent lots of time  On these patches.
> 
> You're right.  I didn't notice this change from v1 to v2.  It would have been a
> more efficient code review had I made these comments in v2.
> 
> My comments this time around, as I said, were minor.  Each version of this
> patch series has been an improvement.  Maybe this version is even good
> enough, but there are /always/ comments to be made about code.  It's not
> unusual to see patches on the kernel mailing list with double digit reroll
> counts (I saw one at v12 today).  Sure, it can be frustrating.   Please don't take
> offense.  Thank you for your efforts.
> 
> The bad news:  I have two more comments.
> 
> Please run scritps/checkpatch.pl --strict with your patch.  There are some
> formatting issues.
> Please add your name to the MAINTANERS file.  Add a section, NTB AMD
> DRIVER.  The checkpatch script also mentions this, but it's especially
> important.  You are the only one who can test this driver, with the simulator.

Ok, I'll do it. 

> The good news:
> 
> I'm inclined to say yes to this patch.  It looks ok, and I'll take your word that it
> has been tested.  Ultimately, the patch has to be accepted by Jon and Linus.
> By adding my comments, I'm just trying to help.
> 
> Do you think you can email v4 by tomorrow?  I'll be sure to respond quickly if
> you do, so it has the best chance to meet the deadline for the merge window.
> It's getting close to the deadline, and I'll take some of the blame for that.

Because I was busying other things at weekend, I'll change it right now.