davidker / unisys Goto Github PK

This is defined in visorbus.h, but never used. Probably should either remove it figure out why it was there in the first place and do that.

301          /* specify the event that has to be triggered when this */
302          /* cmd is complete */
303          cmdrsp->scsitaskmgmt.notify_handle = (u64)&notifyevent;
304          cmdrsp->scsitaskmgmt.notifyresult_handle = (u64)&notifyresult;

Source: Dan Carpenter [email protected] Mon 5/2/2016 5:53 AM

This casting an int pointer to u64 pointer seems bad.

See KanBoard-994

2126          err = visorbus_register_visor_driver(&visornic_driver);
2127          if (!err)
2128                  return 0;

Source: Dan Carpenter [email protected] Mon 5/2/2016 5:53 AM

Flip this around.

if (err)
    goto cleanup_debugfs;

  return 0;

See KanBoard-1005

See KanBoard 806 - http://ustr-erl-6690.na.uis.unisys.com/?controller=task&action=show&task_id=806&project_id=37

drivers/staging/unisys/visornic/visornic_main.c:377 visornic_serverdown() error: double unlock 'spin_lock:&devdata->priv_lock'
drivers/staging/unisys/visornic/visornic_main.c:377 visornic_serverdown() error: double unlock 'irqsave:flags'

This was originally discussed in #24, but Neil agreed that we can fix the immediate problem involving the double-unlock while in staging, and queue the priv_lock removal (#24) until after we get out of staging.

See KanBoad 823: http://ustr-erl-6690.na.uis.unisys.com/?controller=task&action=show&task_id=823&project_id=37.

See also related KanBoard 806: http://ustr-erl-6690.na.uis.unisys.com/?controller=task&action=show&task_id=806&project_id=37.

4/2/2016 1:47 PM Iban Rodriguez:

-----Original Message-----
From: Iban Rodriguez [mailto:[email protected]]
Sent: Saturday, April 02, 2016 1:47 PM
To: Kershner, David A; Greg Kroah-Hartman; Benjamin Romer; Sell, Timothy
C; Neil Horman
Cc: *S-Par-Maintainer; [email protected]; linux-
[email protected]; Iban Rodriguez
Subject: Staging: unisys/verisonic: Correct double unlock

'priv_lock' is unlocked twice. The first one is removed and
the function 'visornic_serverdown_complete' is now called with
'priv_lock' locked because 'devdata' is modified inside.

Signed-off-by: Iban Rodriguez <[email protected]>

---
 drivers/staging/unisys/visornic/visornic_main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c
b/drivers/staging/unisys/visornic/visornic_main.c
index be0d057346c3..af03f2938fe9 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -368,7 +368,6 @@ visornic_serverdown(struct visornic_devdata
*devdata,
        }
        devdata->server_change_state = true;
        devdata->server_down_complete_func = complete_func;
-       spin_unlock_irqrestore(&devdata->priv_lock, flags);
        visornic_serverdown_complete(devdata);
    } else if (devdata->server_change_state) {
        dev_dbg(&devdata->dev->device, "%s changing state\n",

4/2/2016 11:20:PM Tim Sell:

I agree there is a bug here involving priv_lock being unlocked
twice, but this patch isn't the appropriate fix.  Reason is, we can NOT
call visornic_serverdown_complete() while holding a spinlock
(which is what this patch would cause to occur) because
visornic_serverdown_complete() might block when it calls
rtnl_lock() in this code sequence (rtnl_lock() grabs a mutex):

    rtnl_lock();
    dev_close(netdev);
    rtnl_unlock();

Blocking with a spinlock held is always a bad idea.  :-(

4/4/2016 10:35 AM Neil Horman:

You should just get rid of the priv_lock entirely, its not needed.

priv_lock is used the following functions:

visornic_serverdown - only called at the end of a tx_timeout reset operation, so
you are sure that the rx and tx paths are quiesced (i.e. no data access
happening)

visornic_disable_with_timeout - move the netif_stop_queue operation to the top
of this function and you will be guaranteed no concurrent access in the tx path

visornic_enable_with_timeout - same as above, make sure that netif_start_queue
and napi_enable are at the end of the function and you are guarantted no
concurrent access.

visornic_xmit - The queue lock in the netdev_start_xmit routine guarantees you
single access here from multiple transmits.

visornic_xmit_timeout - only called on a tx timeout, when you are guaranteed not
to have concurrent transmit occuing, by definition.

visornic_rx - the only tests made here are to devdata members that are altered
in service_resp_queue, and the visornic_rx is only called from
service_resp_queue, so you are guaranteed a stable data structure, as there is
only ever one context in service_resp_queue as its called from the napi poll
routine

service_resp_queue - Same as above, for any given queue, service_resp_queue only
has one context exectuing at once.

host_side_disappeared - only called from visornic_remove, when implies that all
associated devices are closed already, guaranteeing single access.

visornic_remove 
visornic_resume - Both of these function only get called when all network
interfaces are quiesced.

just remove the lock and make the minor changes needed to guarantee isolated
access.  It makes the code cleaner and faster

Neil

%Lx in format string (no. 1) requires 'unsigned long long' but the argument type is 'unsigned long long'
All the lines that have this issue:
255
277
369
394
403

Why is it here and not in visorchannel or just in the channel itself.

Looking through grok I'm not sure if we ever actually use it. Can we get rid of it?

It's a functiony macro, basically, and it only is the way it is because it's used for both windows and linux requests. Would it be better to add it directly to the do_scsi_nolinuxstat command in visorhba_main? (the only place it's used) Or would it be better as a function.

Current Definition

#define SET_NO_DISK_INQUIRY_RESULT(buf, len, lun, lun0notpresent, notpresent) { \
    MEMSET(buf, 0, MINNUM(len, (unsigned int)NO_DISK_INQUIRY_RESULT_LEN)); \
    buf[2] = (U8)SCSI_SPC2_VER; \
    if(lun == 0) { \
        buf[0] = (U8)lun0notpresent; \
        buf[3] = (U8)DEV_HISUPPORT; \
    } \
    else \
        buf[0] = (U8)notpresent; \
    buf[4] = (U8)(MINNUM(len, (unsigned int)NO_DISK_INQUIRY_RESULT_LEN)-5); \
    if(len >= NO_DISK_INQUIRY_RESULT_LEN) { \
        buf[8] = 'D'; \
        buf[9] = 'E'; \
        buf[10] = 'L'; \
        buf[11] = 'L'; \
        buf[16] = 'P'; \
        buf[17] = 'S'; \
        buf[18] = 'E'; \
        buf[19] = 'U'; \
        buf[20] = 'D'; \
        buf[21] = 'O'; \
        buf[22] = ' '; \
        buf[23] = 'D'; \
        buf[24] = 'E'; \
        buf[25] = 'V'; \
        buf[26] = 'I'; \
        buf[27] = 'C'; \
        buf[28] = 'E'; \
        buf[30] = ' '; \
        buf[31] = '.'; \
    }\
 }

Proposed function

This is an attempt I made in the branch visor_hba_macro_fix. I think the commit would be 7b6b16a

static int set_no_disk_inquiry_result(char * buf, u32 len, u32 lun){
       const char * disk_name;

       disk_name  = "DELLPSEUDO DEVICE .";
       /* len has to be more than the min result length*/
       if (len < NO_DISK_INQUIRY_RESULT_LEN){
               return EINVAL;
       }
       memset(buf, 0, MINNUM(len, NO_DISK_INQUIRY_RESULT_LEN));
       buf[2] = (u8)SCSI_SPC2_VER;
       if (lun) {
               buf[0] = (u8)DEV_NOT_CAPABLE;
       } else {
               buf[0] = (u8)DEV_DISK_CAPABLE_NOT_PRESENT;
               buf[3] = (u8)DEV_HISUPPORT;
       }
       buf[4] = (u8)(MINNUM(len, NO_DISK_INQUIRY_RESULT_LEN) - 5);
       if (len >= NO_DISK_INQUIRY_RESULT_LEN) {
               memcpy(buf + 8, disk_name, strlen(disk_name));
       }
       return 0;
}

Creating this ticket as a result of some observations concerns pointed out by @davidker:

I’m trying to wrap my head around the function controlvm_periodic_work, specifically the variable
poll_count. Is it really just sleeping for the first 250 iterations of the function and then allows it to go
through? Why 250? Why do we even need to sleep that long, to wait for other things to come
through, I thought that was what clientregwait was (right above it). Just the whole process of
poll_count is confusing me at the moment and I’m just waiting for the upstream to ask us about it.

Also what does handle_command_failed do? Are we supposed to error if it fails a second time?
Why are we retrying it and I’m not sure how we every respond with failure if we always fail
handle_command. Do we never actually fail it?

Maybe I’ve been up to long today (had to get up an hour earlier than normal), but I think there is
improvement in this function.

Based upon this excerpt in the https://ustr-svn-1.na.uis.unisys.com/trac/spar/changeset/6443 / r6443 (9 years ago!) log comment, the purpose of poll_count is solely to make testing scenarios work better, therefore I think we can safely remove it:

This changeset also changes the way in which controlvm messages are simulated (for testing
purposes) in the ApplianceOS environment. (The simulation applies when the visorchipset driver is
started with testvnic=1 and testmsg=1, which are currently the default settings.) Previously, the
simulated messages would not start until AFTER the controlvm channel was discovered. For CM2
testing, this meant that you actually needed to start the driver on the host side that was going to
send the controlvm messages. This behavior was changed so that the simulated messages start
automatically about 1 minute after the visorchipset driver is loaded, regardless of whether or not a
controlvm channel is discovered. This provides the added benefit that testing can also be
performed in non-CM2 environments.

I believe handle_command_failed attempts to recover from the scenarios where the controlvm channel queue temporarily fills up, resulting in signalinsert() errors. HOWEVER, we never hit this error condition until we started to use the controlvm channel for file transfer operations (CONTROLVM_TRANSMIT_FILE) in https://ustr-svn-1.na.uis.unisys.com/trac/spar/changeset/20223 / r20223. Of course, these file transfer operations are never used in guest environments, so we could arguably simplify things by removing this retry logic.

296  static ssize_t enable_ints_write(struct file *file,
297                                   const char __user *buffer,
298                                   size_t count, loff_t *ppos)
299  {
300          /*
301           * Don't want to break ABI here by having a debugfs
302           * file that no longer exists or is writable, so
303           * lets just make this a vestigual function
304           */
305          return count;
306  }

Source: Dan Carpenter [email protected] Mon 5/2/2016 5:53 AM

We don't care about ABI so much as we do about applications or scripts.
This was added by Neil Horman because the original code was useless and
dangerous. It was the right patch for him, because he doesn't know
about the userspace so he can't say if anything will break. But
presumably you guys know. Does anything break if we remove this debugfs
file?

Also if so then shame shame shame. User space is not supposed to depend
on debugfs being writable. Just delete this...

See KanBoard-1000

1154  /**
1155   *      visornic_rx - Handle receive packets coming back from IO Part
1156   *      @cmdrsp: Receive packet returned from IO Part
1157   *
1158   *      Got a receive packet back from the IO Part, handle it and send
1159   *      it up the stack.
1160   *      Returns void

Source: Dan Carpenter [email protected] Mon 5/2/2016 5:53 AM

Actually it returns either zero or one but in a convoluted way. Update
it to use literals and update the comment.

See KanBoard-1002

The visorchannel struct both has nbytes and bytes. Both are set to the allocated bytes for the channel. They seem to be used interchangeably. Why?

Source: Dan Carpenter [email protected] Mon 5/2/2016 5:53 AM

del_scsipending_ent()
flip the if condition around so it's error handling instead of success
handling. Remove the "sent = NULL" initializer.

See KanBoard-993

Source: Dan Carpenter [email protected] Mon 5/2/2016 5:53 AM

Still a lot of return -1;
grep "return -1;" drivers/staging/unisys/ -R

See KanBoard-992

From: Greg KH [mailto:[email protected]]
Sent: Wednesday, March 16, 2016 10:07 PM
To: Gavin O'Leary [email protected]
Cc: Kershner, David A [email protected]; Romer, Benjamin M [email protected]; *S-Par-Maintainer [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] drivers: staging: unisys: visornic: Fixed block comment coding style warnings

On Wed, Mar 16, 2016 at 07:01:51PM -0700, Gavin O'Leary wrote:

Fixed block comment checkpatch warnings.

Signed-off-by: Gavin O'Leary [email protected]

drivers/staging/unisys/visornic/visornic_main.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 0519470..eefbacc 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -1315,12 +1315,14 @@ visornic_rx(struct uiscmdrsp cmdrsp)
}
if (found_mc)
break; / accept packet, dest

•                    matches a multicast
  

•                    address */
  

•                  \* matches a multicast
  

•                  \* address
  

•                  */
  

That looks horrible, don't you think?

      }
  } else if (skb->pkt_type == PACKET_HOST) {
      break;  /* accept packet, h_dest must match vnic

•            mac address */
  

•          \* mac address
  

•          */
  

Same here.

Please fix up to look better, that's your goal, right?

thanks,

greg k-h

Use this format to write the comments for struct putfile_active_buffer

Example kernel-doc data structure comment.

/**

• struct blah - the basic blah structure
• @mem1: describe the first member of struct blah
• @mem2: describe the second member of struct blah,

•  perhaps with more lines and words.
  
• Longer description of this structure.
  */

Source: Dan Carpenter [email protected] Mon 5/2/2016 5:53 AM

process_disk_notify()
switch success handling to error handling.

See KanBoard-996

Pulled this from the for-later branch (since interrupt todo's have been done). Determine if we need to do them and then write stories, if we don't need them get rid of the TODOs.

kershnda@USTR-ERL-4458[]:/kernel/master/unisys/drivers/staging/unisys$ find . -print | xargs grep -r -n TODO
./visorbus/visorchipset.c:435: PARSERSTRING_NAME, /* TODO: only PARSERSTRING_NAME is used ? /
./TODO:1:TODO:
./visorhba/visorhba_main.c:604: * TODO: Make this per vhba
./include/guestlinuxdebug.h:138:/ TODO-> Info currently doesn't show, so we set info=warning /
./visorbus/visorchipset.c:435: PARSERSTRING_NAME, / TODO: only PARSERSTRING_NAME is used ? /
./visorbus/visorchipset.c:435: PARSERSTRING_NAME, / TODO: only PARSERSTRING_NAME is used ? /
./TODO:1:TODO:
./visorhba/visorhba_main.c:604: * TODO: Make this per vhba
./visorhba/visorhba_main.c:604: * TODO: Make this per vhba
./include/guestlinuxdebug.h:138:/ TODO-> Info currently doesn't show, so we set info=warning /
./include/guestlinuxdebug.h:138:/ TODO-> Info currently doesn't show, so we set info=warning */
kershnda@USTR-ERL-4458[]:/kernel/master/unisys/drivers/staging/unisys$

char s[99]
x
t

Take care of these.

Needs discussion. What's a doable approach for this that still allows us to do small patches?

`static ssize_t chipsetready_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
char msgtype[64];

    if (sscanf(buf, "%63s", msgtype) != 1)
            return -EINVAL;

    if (!strcmp(msgtype, "CALLHOMEDISK_MOUNTED")) {
            chipset_events[0] = 1;
            return count;
    } else if (!strcmp(msgtype, "MODULES_LOADED")) {
            chipset_events[1] = 1;
            return count;
    }
    return -EINVAL;

}
`

Do we really need this in a guest? Isn't CALLHOMEDISK_MOUNTED just a Service Partition call? Modules_LOADED is that still needed when it gets loaded on demand?

%d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'.
Line with issue
436

https://github.com/davidker/unisys/blob/staging-next/drivers/staging/unisys/Documentation/proc-entries.txt

1019   */
1020  static void
1021  visornic_set_multi(struct net_device *netdev)
1022  {
1023          struct uiscmdrsp *cmdrsp;
1024          struct visornic_devdata *devdata = netdev_priv(netdev);
1025  
1026          /* any filtering changes */
1027          if (devdata->old_flags != netdev->flags) {
1028                  if ((netdev->flags & IFF_PROMISC) !=
1029                      (devdata->old_flags & IFF_PROMISC)) {

Source: Dan Carpenter [email protected] Mon 5/2/2016 5:53 AM

Reverse these if statements and pull the code in two indent levels.

See KanBoard-1001

See KanBoard-803.

I addressed this with commit 9f7702c. checkpatch and regression tested clean.

Defined in visorbus.h and never used.

Source: Dan Carpenter [email protected] Mon 5/2/2016 5:53 AM

visorhba_init()
return a literal zero on success.

See KanBoard-998

There are lot of these in visornic with smatch. I'm wondering what is going on here.

Dan's complaint is specifically about rcvbuf, although it appears to me that we have the same issue for many of our other error-exit cases.

1817          err = visorbus_read_channel(dev, channel_offset,
1818                                      &devdata->num_rcv_bufs, 4);
1819          if (err) {
1820                  dev_err(&dev->device,
1821                          "%s failed to get #rcv bufs from chan (%d)\n",
1822                          __func__, err);
1823                  goto cleanup_netdev;
1824          }
1825  
1826          devdata->rcvbuf = kcalloc(devdata->num_rcv_bufs,
1827                                    sizeof(struct sk_buff *), GFP_KERNEL);
1828          if (!devdata->rcvbuf) {
1829                  err = -ENOMEM;
1830                  goto cleanup_rcvbuf;

Source: Dan Carpenter [email protected] Mon 5/2/2016 5:53 AM

We never allocated rcvbuf. It should still be goto cleanup_netdev; The
other gotos after this in this function are slightly off as well. But
thanks for the nice label names because it makes reviewing this much
easier. :)

See KanBoard-1004

212  static int
213  visor_copy_fragsinfo_from_skb(struct sk_buff *skb, unsigned int firstfraglen,
214                                unsigned int frags_max,
215                                struct phys_info frags[])
216  {
217          unsigned int count = 0, ii, size, offset = 0, numfrags;
                                     ^^

Source: Dan Carpenter [email protected] Mon 5/2/2016 5:53 AM

The second "i" character indicates that it is an integer. Which is ugly
and, of course, misleading.

See KanBoard-999

Even though we do thrinfo->id = 0 in the error path, this isn't enough to prevent the code from accessing thrinfo->task later on, even though it is an invalid value.

Source: Dan Carpenter [email protected] Mon 5/2/2016 5:53 AM

visor_thread_start():
I'm nervous about the error handling here. Is setting ->id to zero
really sufficient to prevent dereferencing ->task?

See KanBoard-991

Both of them are uuids that look about the same in the same structure. I'll remove one and see if it compiles, but I can't seem to find where "type" is set.

DanC's smatch run says:

drivers/staging/unisys/visorbus/visorchipset.c:669 remaining_steps_show() warn: XXX passing uninitialized 'remaining_steps

Needs a patch.

Source: Dan Carpenter [email protected] Mon 5/2/2016 5:53 AM

473                  int old_state = (test_bit(led, visorinput_dev->led));

Remove extra parens.

See KanBoard-1007

The function '__check_debugref' is never used
The function '__check_forcenomatch' is never used
The function '__check_forcematch' is never used
The function '__check_debug' is never used
struct or union member 'channel_size_info::max_size' is never used.
struct or union member 'channel_size_info::min_size' is never used

Source: Dan Carpenter [email protected] Mon 5/2/2016 5:53 AM

I must be blind. How does queue_disk_add_remove() work? Where do we
initialize dar_work_queue?

See KanBoard-995

Source: Dan Carpenter [email protected] Mon 5/2/2016 5:53 AM

visorhba_probe().
We really can only probe one device right? Because VISORHBA_OPEN_MAX is

1. So what happens if we try to probe two? I don't immediately see how
   that is handled.

See KanBoard-997

Do what the subject says for all files.

Do a: grep "rc = -1" drivers/staging/unisys/ -R

Fix these.

(rc != 0) needs to go, from everywhere

struct or union member 'putfile_active_buffer::pnext' is never used.
struct or union member 'putfile_request::file_request_number' is never used.
struct or union member 'putfile_request::data_sequence_number' is never used.
The function '__check_major' is never used.
The function '__check_visorbusregwait' is never used.
The function '__check_visorbusregwait' is never used.

1361  /**
1362   *      devdata_initialize      - Initialize devdata structure
1363   *      @devdata: visornic_devdata structure to initialize
1364   *      #dev: visorbus_deviced it belongs to
1365   *
1366   *      Setup initial values for the visornic based on channel and default
1367   *      values.
1368   *      Returns a pointer to the devdata if successful, else NULL
1369   */
1370  static struct visornic_devdata *
1371  devdata_initialize(struct visornic_devdata *devdata, struct visor_device *dev)
1372  {
1373          if (!devdata)
1374                  return NULL;

Source: Dan Carpenter [email protected] Mon 5/2/2016 5:53 AM

This check doesn't make sense and the error handling is kind of odd.
Just remove it, remove the error handling and update the comment.

See KanBoard-1003

It has a wmb(), why? Shouldn't the lock be good enough?

Greg yelled at the one in visordriver_probe_device which I'm addressing already.

Source: Dan Carpenter [email protected] Mon 5/2/2016 5:53 AM

465          case KEY_NUMLOCK:
466                  led = LED_NUML;
467                  break;
468          default:
469                  led = -1;

Just make this a direct return.

470                  break;
471          }
472          if (led >= 0) {

Then you can remove this if condition.

See KanBoard-1006

Should just be ARRAY_SIZE(...);

Look up ARRAY_SIZE for fun and see!

This needs to be debated at the very least. The upstream's POV is that these designs are messy and hard to follow and error prone, however there are cases where cleaning up at the point of error doesn't suit them either.

We need to come up with a rule and follow it.